- Notifications
You must be signed in to change notification settings - Fork26.8k
Upgrade to import plugin v2#1101
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The changelog lists some added options as well; we'd want to add those to the relevant rules.
In addition, the imports-first rule should be renamed.
@ljharb updated 😄 I only saw one added option ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM pending this one change
| // https://github.com/benmosher/eslint-plugin-import/blob/master/docs/rules/imports-first.md | ||
| 'import/imports-first':['error','absolute-first'], | ||
| // https://github.com/benmosher/eslint-plugin-import/blob/master/docs/rules/first.md | ||
| 'import/first':['error','absolute-first'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Let's also keep a line that disablesimport/imports-first, in case anyone is overriding it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Added (I think 😄 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM! Because this is a breaking change, I'm going to hold off merging this for the time being.
There was a conflict, rebased now. I think at least the new webpack rule should be activated. Do you want that as a separate PR, or included in this? |
That would also be a breaking change, and should be done in a separate PR/commit after this one is merged. |
Arcanemagus commentedOct 11, 2016
Is there a timeline on when support for this version will be released? I've had to revert 3 PR's updating to this that got carelessly merged already. |
I rebased and updated to 2.0.1, at least |
@Arcanemagus no timeline yet. I'd recommend your tests run |
Uh oh!
There was an error while loading.Please reload this page.
Breaking changes seems to just involve changing defaults, which is already defined in this project, so no difference.
Still a breaking change for this module because of the changed peer I suppose, but rules wise, it's not.