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

Already on GitHub? Sign in to your account

Intellj support: code assist #1714

Closed
clausreinke opened this issue Mar 4, 2017 · 4 comments
Closed

Intellj support: code assist #1714

clausreinke opened this issue Mar 4, 2017 · 4 comments

Comments

@clausreinke
Copy link

Intellij users here will have noticed that the react-scripts approch of not having all dependencies in the main package.json leads to a reduced quality of code assistance in that ide.

If you feel like helping to convince them to change that, have a look at this issue:
https://youtrack.jetbrains.com/issueMobile/WEB-25759

@gaearon
Copy link
Contributor

gaearon commented Mar 4, 2017

cc @prigara

@gaearon
Copy link
Contributor

gaearon commented Mar 5, 2017

I replied in the thread, but also going to copy and paste my answer here.


I think IntelliJ’s behavior is absolutely correct here.

react-scripts doesn't install any dependencies for your app to use, except itself of course. For example, just because it depends on lodash, doesn’t mean you should be free to import lodash in your project. This is extremely fragile because the next patch version of react-scripts can remove lodash or use an arbitrarily different version, thus (potentially subtly) breaking your app.

If you want to use a dependency, you must install it yourself and add it to package.json. npm will make sure that your app gets the version it wants, and if there is a conflict, that react-scripts will also get the version it wants.

I filed a new issue to discuss breaking the build when we encounter undeclared dependencies, to make it clearer this behavior was never supported: #1725.

I hope this helps!

@gaearon gaearon closed this as completed Mar 5, 2017
@clausreinke
Copy link
Author

@gaearon

react-scripts doesn't install any dependencies for your app to use, except itself of course.

The problem is that create-react-app does install dependencies for the app to use, beyond react-scripts.

In your case, that is just react and react-dom, but serious react apps will need many more: react-router, redux, react-redux, redux-thunk, react-router-redux, ...

Whether we are talking about a couple or a dozen 'standard' dependencies, they are breaking the idea that updating react-scripts is the way to update projects generated by create-react-app (which is why we avoid eject), and that forking react-scripts is the way to customize create-react-app.

With a transitive dependencies approach, updating our react-scripts-fork was an easy way to add or update additional dependencies, once we agreed on such changes, keeping the set consistent for all our reactjs projects. Admittedly, we relied on our developers not to use transitive dependencies outside the agreed-on set:-)

Okay, I'm looking at alternatives now, and the easiest seems to be this:

  1. add an "exportedDependencies" field to packages/react-scripts/package.json, with the usual "dependencies", "devDependencies", "optDependencies"

  2. in react-scripts/init.js, copy those exported dependencies into the new project's package.json

module.exports = function(appPath, appName, verbose, originalDirectory, template) {
  var ownPackageJson = require(path.join(__dirname, '..', 'package.json'));
  var ownPackageName = ownPackageJson.name;
  var exportedDependencies = ownPackageJson.exportedDependencies;
  var ownPath = path.join(appPath, 'node_modules', ownPackageName);
  var appPackage = require(path.join(appPath, 'package.json'));
  var useYarn = fs.existsSync(path.join(appPath, 'yarn.lock'));

  // Copy over the exported Dependencies
  appPackage.dependencies = Object.assign({}, appPackage.dependencies, exportedDependencies.dependencies);
  appPackage.devDependencies = Object.assign({}, appPackage.devDependencies, exportedDependencies.devDependencies);
  appPackage.optDependencies = Object.assign({}, appPackage.optDependencies, exportedDependencies.optDependencies);

  // Setup the script rules
  appPackage.scripts = {

That works for new projects, but for updating the react-scripts dependency in an existing project, we still need a react-scripts/update.js, to apply any changes in the exported dependencies (that script could also alert the user to any other changes in the template, which cannot be applied automatically).

@gaearon
Copy link
Contributor

gaearon commented Mar 6, 2017

The problem is that create-react-app does install dependencies for the app to use, beyond react-scripts. In your case, that is just react and react-dom

Yes, and you can update them independently from react-scripts. You can even remove them, and the build will still work. These are runtime dependencies, so they are unrelated to the build process that react-scripts takes care of.

but serious react apps will need many more: react-router, redux, react-redux, redux-thunk, react-router-redux, ...

If you need more dependencies, you run npm install [my-dependency] --save. This is it. They are not related to the build process in any way.

they are breaking the idea that updating react-scripts is the way to update projects generated by create-react-app (which is why we avoid eject), and that forking react-scripts is the way to customize create-react-app

Maybe I didn’t explain the idea well enough, but no, they are not breaking that idea. The only concern of react-scripts is providing an environment to develop, build, and test your code. It is not concerned with any React libraries or components. We can’t possibly know which runtime libraries you will need to use.

With a transitive dependencies approach, updating our react-scripts-fork was an easy way to add or update additional dependencies, once we agreed on such changes, keeping the set consistent for all our reactjs projects. Admittedly, we relied on our developers not to use transitive dependencies outside the agreed-on set:-)

This is your decision, but it goes against how npm and Node works. For example, your projects will just fail to build with npm 2.

If you really want to enforce this, you should use the peerDependencies field. This way you would force consumers of your fork to install an additional set of libraries with specific versions, and get warnings if they don’t match.

@lock lock bot locked and limited conversation to collaborators Jan 22, 2019
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants