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 internal to default order groups to match docs #635

Closed
wants to merge 1 commit into from
Closed

Add internal to default order groups to match docs #635

wants to merge 1 commit into from

Conversation

mudetroit
Copy link

@mudetroit mudetroit commented Oct 22, 2016

Quick little bug fix we ran across recently. The docs list the default value for order groups as : ["builtin", "external", "internal", "parent", "sibling", "index"].

But that wasn't matched in the code This simply makes it match..

fixes #601

@coveralls
Copy link

coveralls commented Oct 22, 2016

Coverage Status

Coverage remained the same at 94.896% when pulling e7a24ff on mudetroit:default-groups into 2cbd801 on benmosher:master.

@mudetroit mudetroit closed this Oct 22, 2016
@mudetroit
Copy link
Author

Didn't realize this had been discussed previously in #608

@benmosher
Copy link
Member

Yeah, though now that it has been independently re-suggested, I'm tempted to re-discuss.

@jfmengels: after reflecting on this, it feels like users fall into two major groups:

  1. unaffected by this at all because they are using the stock node resolver (most?)
  2. stand to benefit from adding internal because they are using webpack or meteor

@mudetroit, am I correct in assuming you're in the latter group?

@mudetroit
Copy link
Author

@benmosher yes my current team falls into the later group, and I would surmise that a fair number of the webpack community does. Internal modules, are pretty easy to do with webpack, and avoid a whole class of problem with deeply nested relative pathing.

@IanVS
Copy link

IanVS commented Oct 23, 2016

I work with @mudetroit. We have an internal component library which we previously linked into node_modules to allow for simple imports. After switching to webpack, eslint-plugin-import/order started telling us to put those imports after everything else, which was a bit confusing. We've now overridden the default grouping to allow mixed internal and external, but may clean things up later to differentiate between the two now that we can.

If you want to bring the docs and code into alignment with the least disruption for everyone, it seems like you could use the following as a default order:

'import/order': [ 'error', { groups: [ 'builtin', [ 'external', 'internal' ], 'parent', 'sibling', 'index' ] } ]

This is already the de facto default for anyone not using Webpack, and would avoid linting errors when people switch to webpack with internal modules like we did.

@jfmengels
Copy link
Collaborator

after reflecting on this, it feels like users fall into two major groups:

  1. unaffected by this at all because they are using the stock node resolver (most?)
  2. stand to benefit from adding internal because they are using webpack or meteor

Yes, that sounds right. I originally proposed to fix the code #608, but as @ljharb mentionned, as the rule has always been this way since its introduction, this would probably be a breaking change. Therefore I made another PR #623 to fix the docs instead.

I think that the best course of action would be to fix the docs, and change the default settings in the next major release (which we could do sooner or later).

I originally thought that the default groups would be [ 'builtin', 'external', 'internal', 'parent', 'sibling', 'index' ] (the one written in the docs), but as @IanVS mentionned, we could specify merge external and internal. Not sure which one would fit the best to the most users, but we can discuss it. Though maybe in a new issue.

@benmosher
Copy link
Member

@jfmengels actually yeah, a v3 with this + #479 fixed would be ideal, I suppose

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.

Documented defaults for import/order groups are inaccurate
5 participants