-
Notifications
You must be signed in to change notification settings - Fork 96
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
build: ensure that imports sort automatically #2373
Conversation
@FredericEspiau suggestion for your commit message |
Thanks @FredericEspiau for submitting this new ESLint plugin. I'm currently having trouble to run ESLint against all Did you test it via a $ yarn run lint:js
yarn run v1.22.0
$ eslint --fix --quiet --format=pretty "packages" "scripts"
TypeError: Cannot read property 'range' of null
Occurred while linting /home/foo/ovh/manager/packages/components/ng-ovh-contracts/src/full/directive.js:30
at SourceCode.getTokenBefore (/home/foo/ovh/manager/node_modules/eslint/lib/source-code/token-store/index.js:298:18)
at checkSpacingBefore (/home/foo/ovh/manager/node_modules/eslint/lib/rules/template-curly-spacing.js:60:42)
at TemplateElement (/home/foo/ovh/manager/node_modules/eslint/lib/rules/template-curly-spacing.js:119:17)
at /home/foo/ovh/manager/node_modules/eslint/lib/linter/safe-emitter.js:45:58
at Array.forEach (<anonymous>)
at Object.emit (/home/foo/ovh/manager/node_modules/eslint/lib/linter/safe-emitter.js:45:38)
at NodeEventGenerator.applySelector (/home/foo/ovh/manager/node_modules/eslint/lib/linter/node-event-generator.js:254:26)
at NodeEventGenerator.applySelectors (/home/foo/ovh/manager/node_modules/eslint/lib/linter/node-event-generator.js:283:22)
at NodeEventGenerator.enterNode (/home/foo/ovh/manager/node_modules/eslint/lib/linter/node-event-generator.js:297:14)
at CodePathAnalyzer.enterNode (/home/foo/ovh/manager/node_modules/eslint/lib/linter/code-path-analysis/code-path-analyzer.js:634:23)
error Command failed with exit code 2.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command Thanks |
@antleblanc I ran ESlint manually, no with |
@antleblanc okay I've found the issue, it's a known problem between sources: |
Hello @FredericEspiau, what will be the result of the sorting ? Alphabetical, vendor before app files ? |
@JeremyDec automatic, that's all I care about 😄 There is an example in the documentation: https://github.com/lydell/eslint-plugin-simple-import-sort#example |
fe56314
to
d9d1918
Compare
CDS Report build-monorepository#9883.0 ✘
|
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.
build-monorepository
pipeline fails.
Could you please take a look and make sure that we are still able to build the project?
Thanks!
d9d1918
to
f5ccae0
Compare
@antleblanc I've rebased on |
f5ccae0
to
a3aad93
Compare
CDS Report installation#9916.0 ■
|
@FredericEspiau There is a conflict. An interesting thing regarding this plugin is probably to measure the impact regarding the following script: To make sure that it doesn't have any impacts regarding the performance. We also need to make sure that standalone applications still works. |
What's happen with unamed import like |
Signed-off-by: Frederic Espiau <frederic.espiau@corp.ovh.com>
a3aad93
to
1cb39a1
Compare
Thanks for the rebase, it works now (on local). I see some trouble with AngularJS & the state of our codebase : Example: https://github.com/ovh/manager/blob/master/packages/manager/apps/dedicated/client/app/app.module.js#L68 : this line is moved at the end of the file as follow: import moduleName from './app';
export default moduleName; But as we are using AngularJS, imports https://github.com/ovh/manager/blob/master/packages/manager/apps/dedicated/client/app/app.module.js#L71-L159 could use the AngularJS module defined in Maybe a comment could be used to ignore automatic sorting for this case, but how to be sure that running a |
@cbourgois we can't unfortunately, we have to proceed one by one, but most dependencies should be validated automatically |
I'm closing this as I won't be able to handle it before my leave |
Now imports are sorted automatically so you don't have to care about it
Example
⬇️
Before testing
Please
yarn clean && yarn install
because it might fail because of a bug inbabel-eslint
Sources:
Performances
Before
eslint-plugin-simple-import-sort
✨ Done in 236.82s.
After
eslint-plugin-simple-import-sort
✨ Done in 281.92s.
✨ Done in 240.57s.