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

Add webpack 2 support #475

Merged
merged 3 commits into from
Aug 11, 2016
Merged

Add webpack 2 support #475

merged 3 commits into from
Aug 11, 2016

Conversation

taion
Copy link
Contributor

@taion taion commented Aug 7, 2016

Closes #265

Fixed some lint failures in the plugin along the way.

I don't have test coverage because it seems like it'd be a bunch of additional work to scaffold it into place, because it would require tooling to install dependencies for the test fixtures.

I tested manually and it works on my project, though.

} catch (e) {
// Something has gone wrong (or we're in a test). Use our own bundled
// enhanced-resolve.
webpackRequire = require
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should never hit this in real use given the peer dependency. It's really just for tests.


var SyncNodeJsInputFileSystem = require('enhanced-resolve/lib/SyncNodeJsInputFileSystem')
var syncFS = new SyncNodeJsInputFileSystem()
return EnhancedResolve.create.sync(assign({}, resolveConfig, webpack2DefaultResolveConfig))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah... this plus the config defaults below are (I think) all it takes. Neat.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the order of resolveConfig and webpack2DefaultResolveConfig should be reversed, so that the custom resolveConfig overrides the values on webpack2DefaultResolveConfig. Great job!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, you're right.

@coveralls
Copy link

coveralls commented Aug 7, 2016

Coverage Status

Coverage remained the same at 97.648% when pulling d14ad18 on taion:webpack-2 into 04ded19 on benmosher:master.

@taion taion mentioned this pull request Aug 7, 2016
@coveralls
Copy link

coveralls commented Aug 7, 2016

Coverage Status

Coverage remained the same at 97.648% when pulling d14ad18 on taion:webpack-2 into 04ded19 on benmosher:master.

var resolver = new Resolver(syncFS)
function createWebpack1ResolveSync(webpackRequire, resolveConfig, plugins) {
var Resolver = webpackRequire('enhanced-resolve/lib/Resolver')
var SyncNodeJsInputFileSystem = webpackRequire('enhanced-resolve/lib/SyncNodeJsInputFileSystem')
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/webpack/enhanced-resolve/blob/v0.9.1/lib/SyncNodeJsInputFileSystem.js

This file system object has no caching attached to it AFAICT, so the penalty for constructing it on-the-fly should be trivial.

@taion
Copy link
Contributor Author

taion commented Aug 8, 2016

This diff looks worse than it is. It really just does:

  1. Use the resolve trick to make a webpackRequire that requires on behalf of webpack
  2. Check the version of enhanced-resolve to see which branch to take
  3. Move the webpack 1 imports into the function for creating the webpack 1 resolver, and use webpackRequire there
  4. Add the webpack 2 resolver, which is trivial because of the new ResolverFactory, plus the default config
  5. Shim resolveSync to have the same signature between both versions

@coveralls
Copy link

coveralls commented Aug 8, 2016

Coverage Status

Coverage remained the same at 97.648% when pulling 9a490a9 on taion:webpack-2 into 04ded19 on benmosher:master.

@taion taion mentioned this pull request Aug 8, 2016
@coveralls
Copy link

coveralls commented Aug 8, 2016

Coverage Status

Coverage remained the same at 97.648% when pulling 3babc1b on taion:webpack-2 into 04ded19 on benmosher:master.

@benmosher
Copy link
Member

@taion thanks for the detailed breakdown!

@Jessidhia
Copy link

Trying taion's branch on a small webpack2 project, after removing the root and fallback compatibility keys from my webpack config's resolve:

$ git clone https://github.com/taion/eslint-plugin-import
...
$ rm -rf node_modules/eslint-import-resolver-webpack
$ ln -s ../eslint-plugin-import/resolvers/webpack node_modules/eslint-import-resolver-webpack
$ $(npm bin)/eslint src | grep no-unresolved | wc -l
      15
$ cd eslint-plugin-import && git checkout webpack-2 && cd ..
Branch webpack-2 set up to track remote branch webpack-2 from origin.
Switched to a new branch 'webpack-2'
$ $(npm bin)/eslint src | grep no-unresolved | wc -l
       0

@benmosher
Copy link
Member

@Kovensky if I'm reading this right, you lost 15 no-unresolved reports.

is that.. good? i.e. were there 15 wrong reports that were fixed, or are 15 correct reports now missing?

@Jessidhia
Copy link

@benmosher there were 15 wrong reports that were fixed. The wrong reports were happening even for relative imports (!) or things that could be found inside a node_modules a few dirs up. Using the webpack-2 branch resolves everything correctly.

benmosher added a commit that referenced this pull request Aug 11, 2016
@benmosher benmosher merged commit 3babc1b into import-js:master Aug 11, 2016
@benmosher
Copy link
Member

Merged and published as v0.5.0 to npm.

@taion, thanks again!

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

Successfully merging this pull request may close these issues.

Webpack 2 support
5 participants