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

[WIP] Refuse to build if user is importing something that isn't declared in package.json #1752

Closed
wants to merge 4 commits into from

Conversation

matoilic
Copy link
Contributor

@matoilic matoilic commented Mar 7, 2017

This is a try to implement #1725. Currently it only enables the import/no-extraneous-dependencies ESLint rule. The challenge is now to find a way to force eslint-loader to revalidate invalid files when the package.json changes, e.g. when the user fixes a transitive dependency issue by adding the package to his or her own package.json.

@gaearon
Copy link
Contributor

gaearon commented Mar 7, 2017

The challenge is now to find a way to force eslint-loader to revalidate invalid files when the package.json changes, e.g. when the user fixes a transitive dependency issue by adding the package to his or her own package.json.

That’s the fun part. 👍

@nfcampos
Copy link

nfcampos commented Mar 7, 2017

i believe something along these lines should do the trick

class WatchPackageJson {
  apply(compiler) {
    compiler.plugin("emit", function(compilation, cb) {
      compilation.fileDependencies.push(paths.appPackageJson)
      cb()
    })
  }
}

@matoilic
Copy link
Contributor Author

matoilic commented Mar 7, 2017

TL;DR; To achieve what we want Webpack would need to extend the public API for loaders and eslint-loader would need to change its caching mechanism.


Above code only adds package.json to the list of watched files. It will trigger a new compilation when changed, but only loaders related to JSON files will be affected. eslint-loader will not be invoked by a change to package.json.

Due to the internal caching eslint-loader does

https://github.com/MoOx/eslint-loader/blob/master/index.js#L41

I don't think it's possible to achieve the desired behavior through a Webpack plugin.

What we can do is change the file timestamp through compilation.fileTimestamps. This causes Webpack to reprocess the file. But eslint-loader caches results based on the file content and takes the cached results again. This is most likely because there is no way to access file timestamps through the public API Webpack provides to loaders.

Since we don't change the real file timestamp, but only the one in Webpack's cache, it is no use trying to read the value from the filesystem itself. To get everything working as desired, Webpack would need to provide loaders with a possibility to access file timestamps and loaders would then need to use those for caching instead of relying on the file contents.

As a next step, I would suggest opening an issue in Webpack to discuss this.

@gaearon
Copy link
Contributor

gaearon commented Mar 7, 2017

Due to the internal caching eslint-loader does

FWIW we did not enable this setting in the end (for unrelated reason). Isn't it off by default?

@matoilic
Copy link
Contributor Author

matoilic commented Mar 7, 2017

You're right. The error overlay was stuck for some reason and that's why I thought the results were cached. Sorry for that, I was too quick with my conclusion there. It's already a bit late where I am 😴.

I'll see how far I get with the compilation.fileTimestamps approach.

@gaearon
Copy link
Contributor

gaearon commented Mar 7, 2017

Don’t lose sleep over it, we can come back to this later 😉
Thanks for your help!

nfcampos added a commit to nfcampos/create-react-app that referenced this pull request Mar 8, 2017
@nfcampos
Copy link

nfcampos commented Mar 8, 2017

let me try again this time with a loader ;)
see master...nfcampos:1752

…ransitive or non-existent dependencies are imported
@matoilic matoilic force-pushed the extraneous-dependencies branch from fcf87a9 to efdb992 Compare March 9, 2017 22:08
@matoilic
Copy link
Contributor Author

matoilic commented Mar 9, 2017

I've added a first version of a plugin which triggers a recheck of files errors when package.json changes. So far it works fine with my existing projects. An additional issue I came across is that if a user removes a dependency by mistake no errors are thrown. We could also take care of that in the plugin. But we would have to trigger a recheck on all source files. Another open point is to configure the ESLint import plugin so that it resolves absolute imports properly.

@nfcampos Thank you for the loader implementation. This is also a possible solution. The only downside here ist that package.json is added to all files as a dependency. This would cause all JS source files to be rechecked when package.json changes. Not only the ones containing errors. Even in the case that there are no issues, all files would be rechecked as soon as you change package.json.

@matoilic matoilic force-pushed the extraneous-dependencies branch 5 times, most recently from 38410f3 to a1ca6d2 Compare March 10, 2017 21:05
… erroneous files when package.json is changed, because it might resolve the issue.
@matoilic matoilic force-pushed the extraneous-dependencies branch from a1ca6d2 to 00e0f58 Compare March 10, 2017 21:54
@Timer Timer mentioned this pull request May 5, 2017
32 tasks
@gaearon
Copy link
Contributor

gaearon commented Jan 9, 2018

How close is this to being finished? Is there more things to do / test?

@gaearon gaearon added this to the 2.0.0 milestone Jan 17, 2018
@gaearon gaearon changed the base branch from master to next January 17, 2018 15:02
@mattfysh
Copy link

@matoilic is this something you're still interested in? thx!

@Timer Timer closed this Sep 26, 2018
@Timer Timer reopened this Sep 26, 2018
@Timer Timer modified the milestones: 2.0.x, 2.x Sep 26, 2018
@gaearon gaearon closed this Nov 1, 2018
@lock lock bot locked and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants