fix: ensure that packages properly specify their dependencies #2285
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
DeepCode's analysis on #631f0d found:
💬 This comment has been generated by the DeepCode bot, installed by the owner of the repository. The DeepCode bot protects your repository by detecting and commenting on security vulnerabilities or other critical issues. |
jrainville
reviewed
Feb 21, 2020
michaelsbradleyjr
force-pushed
the
fix/mislocated-deps
branch
5 times, most recently
from
6630344
to
126c8cd
Compare
michaelsbradleyjr
changed the title
michaelsbradleyjr
force-pushed
the
fix/mislocated-deps
branch
from
126c8cd
to
2126d7d
Compare
jrainville
approved these changes
Feb 25, 2020
| @@ -29,6 +30,7 @@ describe('embark.Events', function () { | |||
|
|
|||
| it('should be able to listen to an event emission once', (done) => { | |||
| events.once(testEventName, ({isTest}) => { | |||
| // eslint-disable-next-line no-unused-expressions | |||
There was a problem hiding this comment.
Just an FYI, you can disable eslint rules for whole files by putting the following at the top of the file
/* eslint no-unused-expressions: 0 */
It's especially useful in tests
There was a problem hiding this comment.
Ah, very nice, will keep that in mind for the future. Also, it will get cleaned up when those tests are eventually migrated to jest.
iurimatias
approved these changes
Feb 25, 2020
Many packages in the monorepo did not specify all of their dependencies; they
were effectively relying on resolution in the monorepo's root
`node_modules`. In a production release of `embark` and `embark[js]-*` packages
this can lead to broken packages.
To fix the problem currently and to help prevent it from happening again, make
use of the `eslint-plugin-import` package's `import/no-extraneous-dependencies`
and `import/no-unresolved` rules. In the root `tslint.json` set
`"no-implicit-dependencies": true`, wich is the tslint equivalent of
`import/no-extraneous-dependencies`; there is no tslint equivalent for
`import/no-unresolved`, but we will eventually replace tslint with an eslint
configuration that checks both `.js` and `.ts` files.
For `import/no-unresolved` to work in our monorepo setup, in most packages add
an `index.js` that has:
```js
module.exports = require('./dist'); // or './dist/lib' in some cases
```
And point `"main"` in `package.json` to `"./index.js"`. Despite what's
indicated in npm's documentation for `package.json`, it's also necessary to add
`"index.js"` to the `"files"` array.
Make sure that all `.js` files that can and should be linted are in fact
linted. For example, files in `packages/embark/src/cmd/` weren't being linted
and many test suites weren't being linted.
Bump all relevant packages to `[email protected]`.
Fix all linter errors that arose after these changes.
Implement a `check-yarn-lock` script that's run as part of `"ci:full"` and
`"qa:full"`, and can manually be invoked via `yarn cylock` in the root of the
monorepo. The script exits with error if any specifiers are found in
`yarn.lock` for `embark[js][-*]` and/or `@embarklabs/*` (with a few exceptions,
cf. `scripts/check-yarn-lock.js`).
michaelsbradleyjr
force-pushed
the
fix/mislocated-deps
branch
from
2126d7d
to
631f0da
Compare
michaelsbradleyjr
added a commit
that referenced
this pull request
Mar 11, 2020
This is a pattern established in #2285
iurimatias
pushed a commit
that referenced
this pull request
Mar 11, 2020
This is a pattern established in #2285
iurimatias
added a commit
that referenced
this pull request
Mar 11, 2020
feat: support selecting what library to generate artifacts feat: support selecting what library to generate artifacts feat: support selecting what library to generate artifacts working web3 artifacts remove unnecessary request address code review issues fixes update tests WIP: add index.js in packages/plugins/embarkjs/ This is a pattern established in #2285 remove comment
iurimatias
added a commit
that referenced
this pull request
Mar 12, 2020
feat: support selecting what library to generate artifacts feat: support selecting what library to generate artifacts feat: support selecting what library to generate artifacts feat: support selecting what library to generate artifacts working web3 artifacts remove unnecessary request address code review issues fixes update tests WIP: add index.js in packages/plugins/embarkjs/ This is a pattern established in #2285 remove comment fix some code review issues
iurimatias
added a commit
that referenced
this pull request
Mar 12, 2020
feat: support selecting what library to generate artifacts feat: support selecting what library to generate artifacts feat: support selecting what library to generate artifacts feat: support selecting what library to generate artifacts working web3 artifacts remove unnecessary request address code review issues fixes update tests WIP: add index.js in packages/plugins/embarkjs/ This is a pattern established in #2285 remove comment fix some code review issues
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Many packages in the monorepo did not specify all of their dependencies; they were effectively relying on resolution in the monorepo's root
node_modules. In a production release ofembarkandembark[js]-*packages this can lead to broken packages.To fix the problem currently and to help prevent it from happening again, make use of the
eslint-plugin-importpackage'simport/no-extraneous-dependenciesandimport/no-unresolvedrules. In the roottslint.jsonset"no-implicit-dependencies": true, wich is the tslint equivalent ofimport/no-extraneous-dependencies; there is no tslint equivalent forimport/no-unresolved, but we will eventually replace tslint with an eslint configuration that checks both.jsand.tsfiles.For
import/no-unresolvedto work in our monorepo setup, in most packages add anindex.jsthat has:And point
"main"inpackage.jsonto"./index.js". Despite what's indicated in npm's documentation forpackage.json, it's also necessary to add"index.js"to the"files"array.Make sure that all
.jsfiles that can and should be linted are in fact linted. For example, files inpackages/embark/src/cmd/weren't being linted and many test suites weren't being linted.Bump all relevant packages to
[email protected].Fix all linter errors that arose after these changes.
Implement a
check-yarn-lockscript that's run as part of"ci:full"and"qa:full", and can manually be invoked viayarn cylockin the root of the monorepo. The script exits with error if any specifiers are found inyarn.lockforembark[js][-*]and/or@embarklabs/*(with a few exceptions, cf.scripts/check-yarn-lock.js).