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

Support ESLint v6 #1393

Merged
merged 9 commits into from Jun 24, 2019
Merged

Support ESLint v6 #1393

merged 9 commits into from Jun 24, 2019

Conversation

sheepsteak
Copy link
Contributor

@sheepsteak sheepsteak commented Jun 23, 2019

The main issues are:

To test:

npm i eslint@6
npm test

The only failing tests should be related to the TypeScript issue above.

Related to airbnb/javascript#2036.
Fixes #1362.

@sheepsteak
Copy link
Contributor Author

It's also having some sort of whitespace issue with a couple of tests as you can see here - https://ci.appveyor.com/project/benmosher/eslint-plugin-import/builds/25480510/job/i7a5k9x6j6s1hg8a#L3072. I think I've fixed it and then it comes back again haha

@sheepsteak sheepsteak mentioned this pull request Jun 23, 2019
17 tasks
@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.803% when pulling c61fad0 on sheepsteak:eslint-6 into fc65509 on benmosher:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.803% when pulling c61fad0 on sheepsteak:eslint-6 into fc65509 on benmosher:master.

@coveralls
Copy link

coveralls commented Jun 23, 2019

Coverage Status

Coverage increased (+0.1%) to 97.953% when pulling 7e41d29 on sheepsteak:eslint-6 into fc65509 on benmosher:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.803% when pulling c61fad0 on sheepsteak:eslint-6 into fc65509 on benmosher:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.803% when pulling c61fad0 on sheepsteak:eslint-6 into fc65509 on benmosher:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.803% when pulling c61fad0 on sheepsteak:eslint-6 into fc65509 on benmosher:master.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

It seems like you made a bunch of unrelated formatting changes; please revert them.

src/rules/no-unused-modules.js Outdated Show resolved Hide resolved
src/rules/no-unused-modules.js Outdated Show resolved Hide resolved
src/rules/no-unused-modules.js Outdated Show resolved Hide resolved
src/rules/no-unused-modules.js Outdated Show resolved Hide resolved
tests/src/rules/default.js Show resolved Hide resolved
tests/src/rules/default.js Outdated Show resolved Hide resolved
@ljharb
Copy link
Member

ljharb commented Jun 23, 2019

Are you perhaps running prettier on autosave in your editor or something? If so, disable that - prettier should only be run via eslint autofix, and only on repos that use it.

package.json Outdated Show resolved Hide resolved
@@ -15,7 +15,10 @@ try {
var FileEnumerator = require('eslint/lib/cli-engine/file-enumerator').FileEnumerator
listFilesToProcess = function (src) {
var e = new FileEnumerator()
return Array.from(e.iterateFiles(src))
return Array.from(e.iterateFiles(src), ({ filePath, ignored })=>({
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return Array.from(e.iterateFiles(src), ({ filePath, ignored })=>({
return Array.from(e.iterateFiles(src), ({ filePath, ignored }) => ({

@sheepsteak
Copy link
Contributor Author

sheepsteak commented Jun 23, 2019

Sorry about the double quote changes. I've reverted all of them. I have Prettier set to only work when there is a .prettierrc present so I'm not sure what's causing that 🤷‍♂️

@golopot
Copy link
Contributor

golopot commented Jun 23, 2019

I suppose it is eslint in your editor doing that changes. The ci does not lint over ./tests, so there are many lines in ./tests not passing eslint.

@sheepsteak
Copy link
Contributor Author

sheepsteak commented Jun 23, 2019

Added a commit to only run tests for typescript-eslint-parser when ESLint is <6 where appropriate given that it's archived and will probably never get the required fix.

Would you like me to update Travis CI config to use 6.0.0 instead of 6.0.0-alpha?

@ljharb
Copy link
Member

ljharb commented Jun 23, 2019

Yes, please.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

LGTM pending a nit

@ljharb ljharb self-assigned this Jun 23, 2019
@sheepsteak
Copy link
Contributor Author

sheepsteak commented Jun 23, 2019

@ljharb OK I had to do one more thing and make testVersion take a function instead of an object so that require.resolve didn't get run and throw an error. This happens when trying to resolve the TypeScript parsers on test runs where they've been removed in dep-time-travel.sh. All tests pass on Travis now 🎉

@frederikhors
Copy link

frederikhors commented Jun 24, 2019

Please @ljharb can you merge and release this? This is urgent now with eslint 6 in production everywhere (unfortunately).

"eslint": "2.x - 6.x"

@ljharb
Copy link
Member

ljharb commented Jun 24, 2019

@frederikhors If npm ls exits nonzero, your dep graph is invalid, so eslint 6 shouldn't be in production when you have peer dep conflicts with it. If it's urgent for you, it's a problem of your own making.

@ljharb ljharb merged commit c8132f2 into import-js:master Jun 24, 2019
@frederikhors
Copy link

@ljharb the problem is not mine but it is here: prettier/prettier-vscode#672

@ljharb
Copy link
Member

ljharb commented Jun 24, 2019

Their problem is the same; upgrading to eslint v6 prior to peer deps upgrading is invalid.

@frederikhors
Copy link

@ljharb still same problem today.

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.

support for eslint 6
5 participants