Skip to content

Work around ESLint plugin discovery issue #866

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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
gaearon opened this issue Oct 7, 2016 · 21 comments
Closed

Work around ESLint plugin discovery issue #866

gaearon opened this issue Oct 7, 2016 · 21 comments

Comments

@gaearon
Copy link
Contributor

gaearon commented Oct 7, 2016

There seems to be no visible progress on eslint/eslint#3458, and since it’s disproportionally affecting our users, maybe we should try doing some hack. The biggest issue is that IDEs can’t discover local ESLint and its plugins because they're inside node_modules/react-scripts/node_modules.

We could try a few things:

  • Ship our own bin script called eslint that launches the "real" eslint with the right NODE_PATH or something (not sure if that would even work)
  • Mess with npm internal state and copy all ESLint folders in node_modules one level higher on npm start. This way it won't work right after installing but should work after the project is first started. Maybe we could even make that a postinstall script for react-scripts.
  • Something else crazy.

Regardless of the chosen solution, I’d rather do a hack and fix it up later than keep telling people to install global packages.

@stoikerty
Copy link
Contributor

I see where you're coming from but wouldn't that leave somebody with a hack to deal with once you eject the app?

@just-boris
Copy link
Contributor

just-boris commented Oct 7, 2016

Mess with npm internal state and copy all ESLint folders in node_modules one level higher on npm start

It can be done much simpler. Just exclude eslint-* packages from bundled dependencies, and they will bubble up to the top folder.

I assume that you support only npm3+ aren't you?

@gaearon
Copy link
Contributor Author

gaearon commented Oct 7, 2016

wouldn't that leave somebody with a hack to deal with once you eject the app?

It should be removed as part of ejecting (we have a way to do that: // @remove-on-eject-begin and // @remove-on-eject-end).

I assume that you are supporting only npm3+ aren't you?

Nope, we support npm2 because that’s what ships with the stable Node right now.

@just-boris
Copy link
Contributor

I expected this kind of answer, but as far as the issue is only about developer satisfaction, can we make it at least working only with npm3?

I am so tempted with this because with npm3 we can solve just with removing some code, rather than adding more weirdness.

@gaearon
Copy link
Contributor Author

gaearon commented Oct 7, 2016

Removing ESLint from bundledDependencies will likely make installs slower.
There’s a reason we use them in the first place.

We could give it a try though.
Would you like to publish dummy react-scripts forks and measure the difference?

@just-boris
Copy link
Contributor

just-boris commented Oct 7, 2016

Yes, I can try to play around it in next few days.

P.S. Are you sure that bundledDependencies make install faster? I have tried to measure it and figured out that npm still downloads some data for them anyway.

@stoikerty
Copy link
Contributor

stoikerty commented Oct 7, 2016

It's also worth saying that if anybody wants to help with the underlying issue, grind some coffee and head over to eslint/eslint#3458 to read (and perhaps propose a solution) as fixing this would benefit everyone who uses eslint with plugins, including create-react-app.

@AsaAyers
Copy link
Contributor

AsaAyers commented Oct 7, 2016

I'm using my own fork of linter-eslint that when it doesn't find a node_modules/eslint, it looks for node_modules/react-scripts/node_modules/eslint and uses it. Obviously this hack only works for Atom and only works for react-scripts.

I thought about adding another key to package.json to point to the version of eslint to use. Something like "eslintLocation": "./node_modules/react-scripts/node_modules/eslint". If editors read that key it wouldn't be specific to react-scripts like my current hack is.

@AsaAyers
Copy link
Contributor

AsaAyers commented Oct 7, 2016

Ship our own bin script called eslint that launches the "real" eslint with the right NODE_PATH or something (not sure if that would even work)

This wont' work for linter-eslint because it doesn't execute the bin file. Since the plugin is already JavaScript and eslint is JavaScript, the plugin imports eslint code and runs it directly.

@just-boris
Copy link
Contributor

@gaearon

Would you like to publish dummy react-scripts forks and measure the difference?

Done. I have posted my results there
#41 (comment)
It seems like something was changed since bundledDependencies were introduced in CRA.

@caesarsol
Copy link

caesarsol commented Oct 22, 2016

What about makingeslint and friends install in the actual devDependencies of the newly created project?
The user would just have to use the project-local eslint binary, ant that's not a bad thing in my opinion.

@fson
Copy link
Contributor

fson commented Oct 30, 2016

It was suggested in eslint/eslint#3458 (comment) that it would be possible to depend on ESLint plugins by changing the config from an ESLint shareable config to an ESLint plugin. Plugins can define both rules and configurations. I'm going to give this a try and open a PR so we can see if this solution would work for us.

@fson
Copy link
Contributor

fson commented Oct 31, 2016

#993 implements the "configuration as a plugin" approach. I'm still having some issues with getting the plugin resolved correctly even with this approach, but it might related to how we use bundledDependencies.

@gaearon
Copy link
Contributor Author

gaearon commented Oct 31, 2016

Feel free to remove bundledDeps.

@fson
Copy link
Contributor

fson commented Oct 31, 2016

We need to figure out a better way to do the end to end tests. The cra.sh script currently adds our own packages as bundledDeps, but that causes problems with plugin discovery because own packages end up being in node_modules/react-scripts/node_modules and others are in node_modules so the latter don't see the former.

Any ideas how to run the e2e experience when developing in a way that uses all the local packages, but doesn't use bundledDeps?

@gaearon
Copy link
Contributor Author

gaearon commented Oct 31, 2016

Just copy folders by hand after npm install? That is, with cp.

@fson
Copy link
Contributor

fson commented Oct 31, 2016

Sounds like that could work. Or maybe symlink so you can even make changes without running the whole script from start again?

@gaearon
Copy link
Contributor Author

gaearon commented Oct 31, 2016

¯_(ツ)_/¯ whatever works

@gaearon
Copy link
Contributor Author

gaearon commented Nov 20, 2016

@fson Do we have any progress on this?

@fson
Copy link
Contributor

fson commented Nov 20, 2016

@gaearon I commented about my concerns about the plugin approach here: #993 (comment).

@gaearon
Copy link
Contributor Author

gaearon commented Mar 7, 2017

This doesn't seem to be a big problem with npm 3 now that we removed bundledDependencies.
Closing.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants