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

Webpack 2 support #265

Closed
msuntharesan opened this issue Apr 25, 2016 · 29 comments · Fixed by #475
Closed

Webpack 2 support #265

msuntharesan opened this issue Apr 25, 2016 · 29 comments · Fixed by #475

Comments

@msuntharesan
Copy link

Resolve options in webpack 2.0 is considerably changed. Any way to support that now?

@benmosher
Copy link
Member

Need a Webpack 2.0 resolver.

Could be a new package (eslint-import-resolver-webpack2) or a new major revision of the existing Webpack resolver, though I'd probably opt for the former as I (and I suspect many others) will likely not make the transition to Webpack 2 anytime soon (if even ever!).

@benmosher
Copy link
Member

FYI, here's the readme section on resolvers: https://github.com/benmosher/eslint-plugin-import#resolvers

The jspm resolver is the only community one I'm aware of so far, as I've been stewarding the Node and Webpack (1) resolvers out of this repo. Each resolver is a package unto itself though, so anyone could write and publish one for Webpack 2.

I'm not planning to implement this any time soon, but a PR with a resolvers/webpack2 package would be welcome. 😄

@benmosher
Copy link
Member

Actually, now that I've read through that section again, I can imagine supporting both at once in the Webpack resolver. Would be worth a shot, anyway, might be simpler than forking or trying to share common code.

@msuntharesan
Copy link
Author

Thanks for the updates.

I beg to defer on your opinion on the adoption of webpack 2. I think webpack 2 will be adopted soon rather than never. Yes the config is a pain but the native support for es6 modules will be worth it.

Looking at the config for webpack 2 and what you have for resolve-alias.js, shouldn't it be possible to do something similar. The issue I see is that the a specific module can occur anywhere in the tree.
Ex:

{
  resolve: {
    extensions: ['', '.jsx', '.scss', '.js', '.json', '.styl', 'ts', 'tsx'],
    modules: [
      'web_modules',
      'node_modules',
      'shared',
      'res'
    ]
  },
}

The shared folder can be in multiple places. My folder tree follows like this

- app
  - master
    - components
    - dashboard
      - components
    - revenue
       - components
    - shared
      - common-components-for-master
  - root
  - services
  - root
  - services
  - shared
    - anything-common-for-app
- config 
- shared
  - components
    - some
    - random
    - shared-components
  - utils

In my case revenue can import something like import AComponent from common-components-for-master from within components, dashboard or revenue. I hope that helps

@benmosher
Copy link
Member

I think webpack 2 will be adopted soon rather than never.

Just to clarify: I assume the community will adopt it pretty immediately, but I don't expect to use it at my day job for a while, if for no other reason than that v1 is working fine. 😄

So I just want to make sure that whatever happens, Webpack v1 support is preserved, that's all.

I think what you've described could be pretty easily supported with some tweaks in place to the existing Webpack resolver, without compromising v1 support.

@benmosher
Copy link
Member

Just a note: want to use this with the Webpack 2 resolver: https://github.com/webpack/enhanced-resolve/blob/master/lib/ResolverFactory.js#L38

Will also need to refactor the path sanitizing (removing loaders and queries) and config-finding into some shared module. Still pretty sure we'll need 2 modules, as the Webpack 2 version will need to find/depend on enhanced-resolve 2.

(and I don't think we can assume the live webpack package will be require-able from the resolver, or we could do it dynamically. might be an ideal to try for and fail back to explicit dependencies... or declare it as a peer dependency?)

@benmosher benmosher modified the milestones: next, soon, 2.0.0 Jun 30, 2016
@nnarhinen
Copy link

nnarhinen commented Jul 5, 2016

btw, according to my tests you can use the webpack1 syntax in your webpack.config.js alongside webpack2 syntax. This way both import eslinter and webpack2 work:

{
  //...
  resolve: {
    modules: [path.resolve('./src'), 'node_modules'],
    root: path.resolve('./src')
  }
}

@taion
Copy link
Contributor

taion commented Jul 26, 2016

@benmosher

Would you accept a PR that makes the package resolution lenient in a hack-ish way that pragmatically gives effective webpack 2 compatibility?

For example, at https://github.com/benmosher/eslint-plugin-import/blob/resolvers/webpack/v0.4.0/resolvers/webpack/index.js#L138, we could do:

resolve.modulesDirectories || resolve.modules || ['web_modules', 'node_modules']

This wouldn't be correct "per se", but it would work.

@Jessidhia
Copy link

You need to filter by relative / absolute paths.

@benmosher
Copy link
Member

@taion would be dependent on the PR details, but I'm not generally opposed.

That said, I'm not sure that making a "real" Webpack 2 resolver is a long walk away, and I would prefer that.

@taion
Copy link
Contributor

taion commented Jul 26, 2016

I agree that it's not. It'd be a way to not have to have an eslint-import-resolver-webpack-2 though.

@benmosher
Copy link
Member

benmosher commented Jul 26, 2016

Another way would be to resolve enhanced-resolve from a sibling webpack, but it would require eslint-plugin-import (or at least the Webpack resolver) and webpack to be able to reach each other, which might be okay but would be a bit restrictive.

Then could branch inside the resolver based on the version in enhanced-resolve's package.json.

Would be a little bit better, potentially, as it would guarantee that the same exact version of enhanced-resolve was used for linting as for building.

@taion
Copy link
Contributor

taion commented Jul 26, 2016

The problem is that enhanced-resolve is a direct dependency rather than a peer dependency of webpack. It would sort of work with npm3 but I don't think you'd be able to access webpack's enhanced-resolve at all with npm2.

@Jessidhia
Copy link

try { require('enhanced-resolve') } catch (e) { require('webpack/node_modules/enhanced-resolve') }

@benmosher
Copy link
Member

benmosher commented Jul 26, 2016

I was imagining using stock require.resolve to find webpack relative to eslint-import-resolver-webpack, then resolving enhanced-resolve using node-resolve relative to webpack, then actually require-ing the resulting absolute path to enhanced-resolve.

A bit convoluted? Sure. But assuming it's >90% correct, it'd be incredibly convenient.

Biggest leap is finding the right webpack, which I suppose could become part of config, but assume it's the one that require.resolve would find from the eslint-import-resolver-webpack install location. If both are devDependencies of the same package.json, that should be easy (literally ../webpack, but require.resolve would be in charge of that).

@benmosher
Copy link
Member

@Kovensky's solution would also probably work a lot of the time.

@taion
Copy link
Contributor

taion commented Aug 7, 2016

Added in #475, using resolve mostly per #265 (comment) (except we can't ever use require.resolve since resolution needs to be relative to the config file).

@benmosher
Copy link
Member

@taion sweet, thanks! I scanned over your PR, but need to take a closer look. Will try to get to it this week.

@Others, if you're able to install @taion's forked Webpack resolver and test it out and report back success/issues, that would be great feedback.

@bensleveritt
Copy link

Anyone know how to install the resolver from a fork? Just installing the fork doesn't seem to work...

@taion
Copy link
Contributor

taion commented Aug 9, 2016

I think you have to clone it and install from the local path.

@rmarscher
Copy link

git clone https://github.com/taion/eslint-plugin-import.git
cd eslint-plugin-import
git checkout webpack-2
cd resolvers/webpack
npm install
pwd

^^^ take the folder output by that final pwd command and put it in your eslintrc settings.

  "extends": "airbnb",
  "root": true,
  "settings": {
    "import/resolver": {
      "/home/me/eslint-plugin-import/resolvers/webpack": {}
    }
  }

You can use the DEBUG=eslint-plugin-import:resolver:webpack to get additional debug output too.

I'm using DEBUG=eslint-plugin-import:resolver:webpack node_modules/.bin/eslint . ./ in my project. It currently fails for me because I'm using a webpack-2 config that exports a function instead of an object literal.

@taion can you try adding some code to check if the config is a function right before line 82 of index.js? Probably need to add a unit test for that use case too.

  // webpack 2 config function support
  if (typeof webpackConfig === 'function') {
    var configOptions = get(settings, 'config-options') || {}
    webpackConfig = webpackConfig(configOptions)
  }

Once I add the above block, it works for my usage of eslint with webpack 2. I took a stab at implementing it too, but I didn't know about recent webpack 1's usage of enhanced-resolve. I thought a separate webpack-2 resolver was going to be necessary. Awesome. Thanks!!

@taion
Copy link
Contributor

taion commented Aug 9, 2016

Wait, what? Does webpack 2 support functions for configs? I've not seen that feature – any details?

@rmarscher
Copy link

@taion
Copy link
Contributor

taion commented Aug 9, 2016

I feel like that might make more sense as a separate PR. Among other things, it'd require additional docs, rather than just "things will now just work for similar webpack 2 configs".

@rmarscher
Copy link

Sure. I could potentially take on that PR after #475 is merged.

@Jessidhia
Copy link

You'd also need a separate option to specify which --env or --env.key options are needed to give to the config function, as it might do the wrong thing if you call it with undefined or {}. Support for function configs is definitely a much bigger scope than just supporting webpack2-style resolve.

@rmarscher
Copy link

My snippet above supports a "config-options" setting in the import/resolver eslint settings and passes that to the function. I think that's a decent solution for most uses.

@benmosher benmosher removed this from the soon milestone Aug 11, 2016
undefinedExpert pushed a commit to undefinedExpert/zxresult that referenced this issue Sep 1, 2016
@benmosher
Copy link
Member

@rmarscher, FYI: just published v0.6.0 of the resolver, it includes support for webpack-config-as-function 😎 via #533

@rmarscher
Copy link

@benmosher That's great. Thanks for remembering to ping me. People might open issues about needing to pass an argument to that webpack function which is what happens in the command-line version when you use the --env arguments. The way I have been dealing with it is to have a separate file that imports the webpack 2 config function, executes it and exports a plain object. Then I use the config setting to point eslint to that file.

"settings": {
  "import/resolver": {
    "webpack": {
      "config": "webpack.config.eslint.js"
    }
  }
}

As @Kovensky pointed out, there are going to be a lot of different ways the function will need to be called for different use cases, so maybe recommending this approach keeps from having to handle all of that within this plugin.

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

Successfully merging a pull request may close this issue.

8 participants