Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃敡 Use require.resolve instead of homegrown methods #12

Merged
merged 4 commits into from Aug 11, 2019

Conversation

@depoulo
Copy link
Contributor

commented Jul 5, 2019

This enables monorepo support

@depoulo depoulo changed the title :wrench: Use require.resolve instead of homegrown methods Use require.resolve instead of homegrown methods Jul 5, 2019

@obahareth

This comment has been minimized.

Copy link
Owner

commented Jul 5, 2019

Hey @depoulo, I like the idea behind this but it's failing with many packages when I run it:

鈿狅笍 uid was not checked because no entry script was found
鈿狅笍 underscore was not checked because no entry script was found

The current code handles cases where the package does not include an entry script, or puts a path to an entry script that doesn't exist (I try to find an index.js in that case).

Is there a way we could use require.resolve for those cases? If not, then is there a way monorepo support could be enabled with the existing code? I haven't dealt too much with monorepos, do you have any examples I could see?

@depoulo

This comment has been minimized.

Copy link
Contributor Author

commented Jul 6, 2019

The current code handles cases where the package does not include an entry script, or puts a path to an entry script that doesn't exist (I try to find an index.js in that case).

I'm not bullet proof regarding module loaders, but do you have an example of a module where this applies (edit: underscore, I see, will look into it)? Wondering how you can require it than using Node or Webpack...

@depoulo

This comment has been minimized.

Copy link
Contributor Author

commented Jul 6, 2019

If not, then is there a way monorepo support could be enabled with the existing code?

You could go for an approach like this:
master...somtoZikora:monorepo-extension-epages

Monorepos contain additional package.json's in <root>/packages/* subfolders, scripts might be run from there, yet node_modules used by the packages can be hoisted to <root>/node_modules.

@obahareth

This comment has been minimized.

Copy link
Owner

commented Jul 7, 2019

@depoulo Have a look at https://github.com/MatthewMueller/uid, lots of packages depend on it.

Also I don't know why it failed with something like underscore because the file does exist.

By the way I enabled CircleCI on PRs, I don't know why it didn't turn on by default.

@depoulo

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2019

鉃  are-you-es5 git:(use-require-resolve) y add uid
yarn add v1.16.0
success Saved 1 new dependency.
info All dependencies
鈹斺攢 uid@0.0.2
鉃  are-you-es5 git:(use-require-resolve) 鉁 node
Welcome to Node.js v12.4.0.
Type ".help" for more information.
> require.resolve('uid')
'/home/paolo/code/are-you-es5/node_modules/uid/index.js'

鉃  are-you-es5 git:(use-require-resolve) 鉁 y add underscore
yarn add v1.16.0
success Saved 1 new dependency.
info All dependencies
鈹斺攢 underscore@1.9.1
鉃  are-you-es5 git:(use-require-resolve) 鉁 node -pe "require.resolve('underscore')"
/home/paolo/code/are-you-es5/node_modules/underscore/underscore.js

馃

I also can't reproduce these errors when I add those dependencies:

鈿狅笍 uid was not checked because no entry script was found
鈿狅笍 underscore was not checked because no entry script was found

Next, I edited the uid entry script (added a default parameter) and got "uid is not ES5."

@depoulo depoulo changed the title Use require.resolve instead of homegrown methods 馃敡 Use require.resolve instead of homegrown methods Jul 8, 2019

@obahareth

This comment has been minimized.

Copy link
Owner

commented Jul 13, 2019

@depoulo editing the entry script isn't something we can do, we have to check the packages as they were when they came in from NPM. Perhaps you could edit this PR to keep some of the homegrown methods that deal with cases like the main script being an invalid path? I really like the idea of using require.resolve but I think it needs a bit of help from the homegrown methods here that deal with weird packages that declare a nonexistent main script or don't declare one at all.

I think the way people use those packages is with destructured imports? e.g. import { Something } from 'some-package'? I'm honestly not sure.

I'm not really sure. I tried again building and running your code against a repo with thousands of node_modules and I got no entry script was found on 930 of them. When I ran it with the code on master branch on the same repo no entry script was found only occurred 51 times. Could there be a difference in environment between our machines that's causing this?

I'll try adding some of these problematic packages to the tests to see if we can at least reproduce this same behavior on CI.

@depoulo

This comment has been minimized.

Copy link
Contributor Author

commented Jul 13, 2019

Require.resolve resolves whatever is in main, or an index.js. We could add a check for invalid main script entries (fs.exists), still I wonder how such a script could be required IRL. As for destructured imports, it's possible to require anything relative to the package root, e.g axios/lib/urlHelpers, or lodash/omit, but the current code also can't check those, yet you assumption (if it's ES6, then the entry script is gonna be ES6 as well) seems valid.
There will always be some uncovered cases, I had one dependency loaded via AMD that was in ES6 recently.

@depoulo

This comment has been minimized.

Copy link
Contributor Author

commented Jul 13, 2019

editing the entry script isn't something we can do

sure, I just did for proving that my code is able to detect a non-ES5 uid module.

@depoulo

This comment has been minimized.

Copy link
Contributor Author

commented Jul 13, 2019

I'll try adding some of these problematic packages to the tests to see if we can at least reproduce this same behavior on CI.

馃憤

@depoulo depoulo force-pushed the depoulo:pull/use-require-resolve branch from 55055f8 to 7e43fdb Jul 17, 2019

obahareth added a commit that referenced this pull request Jul 27, 2019

Add more NPM packages for more accurate testing
Add the following packages to test suite:

- react
- uid
- underscore
- whatwg-fetch

This should help with testing #12

obahareth added a commit that referenced this pull request Jul 27, 2019

Add more NPM packages for more accurate testing
Add the following packages to test suite:

- react
- uid
- underscore
- whatwg-fetch

This should help with testing #12
@obahareth

This comment has been minimized.

Copy link
Owner

commented Jul 27, 2019

@depoulo So I am kind of baffled. I added a lot more tests to replicate the behavior I am seeing on my machine but I can't. Your branch is passing the tests. What's confusing me is that the tests aren't showing the same behavior as the distribution version on my machine.

Here is what I am doing

yarn build
node ./dist/index.js check -av ./tests/support/fixtures/root/

Here's the output I'm getting: (used pastebin cause it's kind of too long)

The repo I am testing on has these dependencies:

{
  "dependencies": {
    "@babel/plugin-transform-block-scoping": "^7.2.0",
    "@babel/plugin-transform-object-assign": "^7.2.0",
    "@babel/plugin-transform-parameters": "7.2.0",
    "@babel/preset-react": "^7.0.0",
    "@babel/register": "^7.0.0",
    "@rails/webpacker": "4.0.0-rc.2",
    "axios": "^0.18.0",
    "babel-plugin-lodash": "^3.3.4",
    "babel-plugin-transform-react-remove-prop-types": "^0.4.21",
    "bson-objectid": "^1.2.3",
    "dayjs": "^1.7.7",
    "draft-js": "^0.10.5",
    "draft-js-alignment-plugin": "^2.0.5",
    "draft-js-anchor-plugin": "^2.0.2",
    "draft-js-autolist-plugin": "^2.0.0",
    "draft-js-block-breakout-plugin": "^2.0.1",
    "draft-js-buttons": "^2.0.1",
    "draft-js-drag-n-drop-plugin": "^2.0.3",
    "draft-js-focus-plugin": "^2.2.0",
    "draft-js-image-plugin": "^2.0.6",
    "draft-js-inline-toolbar-plugin": "^3.0.0",
    "draft-js-linkify-plugin": "^2.0.1",
    "draft-js-plugins-editor": "^2.1.1",
    "draft-js-resizeable-plugin": "^2.0.8",
    "draft-js-side-toolbar-plugin": "^3.0.1",
    "lodash": "^4.17.11",
    "lodash-webpack-plugin": "^0.11.5",
    "moment": "^2.22.2",
    "moment-locales-webpack-plugin": "^1.0.7",
    "object-dig": "^0.1.1",
    "pagination": "^0.4.4",
    "polished": "^2.0.3",
    "postcss-cssnext": "^3.1.0",
    "react": "^16.4.2",
    "react-color": "^2.14.1",
    "react-datepicker": "^1.5.0",
    "react-dom": "^16.4.2",
    "react-helmet": "^5.2.0",
    "react-localization": "^0.1.2",
    "react-placeholder": "^3.0.1",
    "react-popper": "^1.0.2",
    "react-rating": "^1.2.1",
    "react-redux": "^5.0.2",
    "react-router-dom": "^4.2.2",
    "react-router-modal": "^1.2.0",
    "react-scroll": "^1.7.7",
    "react-select": "^1.2.1",
    "react-sortable-hoc": "^0.8.3",
    "react-toastify": "^4.0.1",
    "redux": "^4.0.0",
    "redux-promise-middleware": "^5.1.1",
    "resolve-url-loader": "^2.3.0",
    "uid": "^0.0.2",
    "universal-cookie": "^2.1.5",
    "uppy": "^0.24.4",
    "webpack-merge": "^4.1.0"
  },
  "devDependencies": {
    "babel-eslint": "^8.2.3",
    "caniuse-lite": "^1.0.30000918",
    "chai": "^4.1.2",
    "chai-as-promised": "^7.1.1",
    "core-js": "^2.5.7",
    "dotenv": "^4.0.0",
    "enzyme": "^3.3.0",
    "enzyme-adapter-react-16": "^1.1.1",
    "eslint": "^4.9.0",
    "eslint-config-airbnb": "^17.1.0",
    "eslint-import-resolver-webpack": "^0.10.1",
    "eslint-plugin-import": "^2.14.0",
    "eslint-plugin-jsx-a11y": "^6.1.2",
    "eslint-plugin-react": "^7.11.1",
    "ignore-styles": "^5.0.1",
    "jsdom": "^11.3.0",
    "mocha": "^5.2.0",
    "mocha-junit-reporter": "^1.15.0",
    "moxios": "^0.4.0",
    "react-dev-utils": "^6.1.1",
    "redux-logger": "^3.0.6",
    "shebang-loader": "^0.0.1",
    "sinon": "^4.0.1",
    "webpack-bundle-analyzer": "^2.13.1",
    "webpack-dev-server": "^3.1.4",
    "webpack-notifier": "^1.7.0",
    "webpackbar": "^3.1.4"
  }
}

The strange thing is, when I run it on that repo, it says no entry script was found for react, but when I run it on the fixtures in this repo's tests dir, is doesn't have the same issue with react.

@obahareth

This comment has been minimized.

Copy link
Owner

commented Jul 27, 2019

So even though I can't repeat it in the tests, If I run:

yarn build
node ./dist/index.js check -a ./tests/support/fixtures/root/

I get this on your branch

node ./dist/index.js check -a ./tests/support/fixtures/root/
鈿狅笍 is-even was not checked because no entry script was found
鈿狅笍 is-odd was not checked because no entry script was found
鈿狅笍 prop-types was not checked because no entry script was found
鈿狅笍 react was not checked because no entry script was found
鈿狅笍 react-is was not checked because no entry script was found
鈿狅笍 scheduler was not checked because no entry script was found
鈿狅笍 uid was not checked because no entry script was found
鈿狅笍 underscore was not checked because no entry script was found
鈿狅笍 whatwg-fetch was not checked because no entry script was found

but on master I do not get any errors for those. I need to figure out why the tests are not matching the behavior of the built version.

@depoulo depoulo force-pushed the depoulo:pull/use-require-resolve branch from 7e43fdb to b542b3d Jul 30, 2019

@depoulo

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2019

So I prematurely force pushed a change - now I'll have to check if it still works for monorepos 馃う鈥嶁檪 - guess I should add a test 馃槅

depoulo added some commits Jul 31, 2019

@depoulo

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2019

It should be worth noting that for monorepos you'll have to call are-you-es5 with the subpackage directory as a parameter, but that's the way to go for monorepos. You can always call it for ./packages/* in a shell loop.

@depoulo

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2019

By the nature of how -a works, it won't check dependencies of a monorepo subpackage that are hoisted to the root.

yarn build
鉃  are-you-es5 git:(pull/use-require-resolve) node ./dist/index.js check -v ./tests/support/fixtures/root/packages/some-package
鉁 acorn is ES5
鉁 underscore is ES5
鉃  are-you-es5 git:(pull/use-require-resolve) node ./dist/index.js check -av ./tests/support/fixtures/root/packages/some-package
鉁 underscore is ES5

This could be fixed like so:

diff --git a/src/modules-checker.ts b/src/modules-checker.ts
index fb7e358..6326a27 100644
--- a/src/modules-checker.ts
+++ b/src/modules-checker.ts
@@ -51,7 +51,11 @@ export class ModulesChecker {
     if (!this.config.checkAllNodeModules) {
       return this.getDepsFromRootPackageJson()
     } else {
-      return this.getAllNodeModules()
+      return Array.from(
+        new Set(
+          this.getAllNodeModules().concat(this.getDepsFromRootPackageJson())
+        )
+      )
     }
 
     return null
@obahareth

This comment has been minimized.

Copy link
Owner

commented Aug 1, 2019

@depoulo Do you think -a along with that change gives you the behavior you need? Is that enough for you or do you still wanna try taking out the homegrown methods?

@depoulo

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2019

No no, what I'm describing is the behaviour when using this pull request. So the question is rather, should we also apply the diff I posted, or should we just better document the behaviour of -a (in that it just looks at all deps in the node_modules folder), or should we leave my PR as it is right now?

@obahareth

This comment has been minimized.

Copy link
Owner

commented Aug 4, 2019

@depoulo If the diff you posted works for the previous cases as well as monorepos, let's apply it?

Were you able to check the behavior of require.resolve with some of the packaged I mentioned? Like I mentioned before, I sadly wasn't able to replicate the strange behavior I was seeing in unit tests... I only got failures when actually checking packaged via the CLI.

Fix -a option to really check all node_modules
even in a monorepo setup
@depoulo

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2019

Thank you for your feedback! Feel free to run all checks, everything should be cool now!

@obahareth

This comment has been minimized.

Copy link
Owner

commented Aug 11, 2019

Tried it out, runs well for me now! Well done @depoulo and sorry that this has taken so long!

@obahareth obahareth merged commit fe940d0 into obahareth:master Aug 11, 2019

1 check passed

ci/circleci: build Your tests passed on CircleCI!
Details
@depoulo

This comment has been minimized.

Copy link
Contributor Author

commented Aug 11, 2019

Haha, no problem, and thanks for your patience!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can鈥檛 perform that action at this time.