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

Refuse to build if user is importing something that isn't declared in package.json #1725

Open
gaearon opened this issue Mar 5, 2017 · 21 comments

Comments

@gaearon
Copy link
Contributor

gaearon commented Mar 5, 2017

I think Brunch does this, if I recall correctly.

This is an important feature because people often misunderstand that transitive dependencies aren’t supposed to be importable.

See #1714 and #1622 for examples.

@matoilic
Copy link
Contributor

matoilic commented Mar 5, 2017

eslint-plugin-import also has a rule which could help to hint developers about importing transitive dependencies.

@gaearon
Copy link
Contributor Author

gaearon commented Mar 5, 2017

@jokeyrhyme I don't think these are similar. The rules you link to protect against importing a non-existing file (which would break the build anyway). But I'm talking about a file that exists but comes from a transitive dependency.

@gaearon
Copy link
Contributor Author

gaearon commented Mar 5, 2017

@matoilic This one looks right. Want to send a PR adding it?

@gaearon
Copy link
Contributor Author

gaearon commented Mar 5, 2017

One potential issue here is I don't think editing package.json would trigger an eslint-loader recheck, so the user would be stuck with a misleading message in our setup.

@matoilic
Copy link
Contributor

matoilic commented Mar 5, 2017

@gaearon Sure, I can work out a PR. What do you think about importing dev dependencies? Should there be a warning / error when importing a dev dependency, especially in a production build?

I'm not sure on how to trigger a recheck or rebuild when editing package.json. Could a Webpack plugin be a solution? I haven't come across one which does something like that. We might need to create one to solve the issue.

@mnquintana
Copy link

What would you think about implementing this upstream in webpack? This seems like it'd be super helpful for all / most projects, not only React projects / projects using that ESLint plugin.

@gaearon
Copy link
Contributor Author

gaearon commented Mar 5, 2017

Could a Webpack plugin be a solution?

That may be the case. I’m not sure.

What would you think about implementing this upstream in webpack?

It may be better longer term but if we can prototype this feature here and figure out the edge cases sooner, that might work out just as well. IMO plugins like this end up good when they start in userland, and if proven useful, move into the core.

@gaearon
Copy link
Contributor Author

gaearon commented Mar 5, 2017

@Timer
Copy link
Contributor

Timer commented Mar 5, 2017

I think this is important enough to not tag as a future idea and instead for 0.10. 😄

@gaearon gaearon added this to the 0.10.0 milestone Mar 5, 2017
@gaearon
Copy link
Contributor Author

gaearon commented Mar 5, 2017

Sounds good. I’m not sure I’m aware of a good way to make it happen tho.

@jokeyrhyme
Copy link

@matoilic
Copy link
Contributor

matoilic commented Mar 6, 2017

I think those two rules are more intended for package creators. When you install react-scripts the NPM registry will only contain "published" files and thus only files which already pass the check. That's how I understand it. But those rules might be useful to solve #1720.

@gaearon
Copy link
Contributor Author

gaearon commented May 11, 2017

Pushing back to 0.11.

@Ayc0
Copy link

Ayc0 commented Sep 20, 2017

Any news ?
@matoilic

@mattfysh
Copy link

This would be a great addition for junior engineers who don't understand the reason their import works is due to hoisting and/or node_modules flattening

In the meantime, I'm setting up depcheck

@Timer Timer modified the milestones: 2.0.x, 2.x Sep 26, 2018
@Timer Timer removed this from the 2.x milestone Nov 2, 2018
@stale
Copy link

stale bot commented Dec 2, 2018

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed in 5 days if no further activity occurs.

@stale stale bot added the stale label Dec 2, 2018
@stale
Copy link

stale bot commented Dec 8, 2018

This issue has been automatically closed because it has not had any recent activity. If you have a question or comment, please open a new issue.

@mdaj06
Copy link

mdaj06 commented Sep 2, 2021

@Timer is this open?

@nikikalwar
Copy link

Hi @Timer,

The behaviour is expected because you are trying to import something which is not there.

Let's say If I am importing with the below code:

import {debounceTime} from 'rxjs';

so, for me to access the debounceTime Property from rxjs, i need to have the rxjs object right? so as you said earlier that the build is getting refused because user is importing that isn't declared in the package.json and whether it is declared it doesn't matter.

I imported rxjs which was not declared and build failed so, I installed the package, deleted it's entry from package.json and ran the app with no errors along with the import statement.

The thing is we are importing from node_modules not from package.json so, in the build.js file we can add a warning message saying , the $package is not installed and suggest the user to install the package instead of just saying the below error:

Failed to compile.

Module not found: Error: Can't resolve 'rxjs' in 'F:\github\test3\my-app\src'

Why? Because with the module not found is kind of too vague as a user warning

else { console.log(chalk.red('Failed to compile.\n')); printBuildError(err); console.warn(" Please install the unresolved dependency and then run npm build") process.exit(1);

@BraianS
Copy link

BraianS commented Apr 24, 2024

Good night.

Any updates for this issue? If not, this may resolve the issue.
Thanks.

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

No branches or pull requests

10 participants