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

[New] Support for ESLint v8 #2191

Merged
merged 1 commit into from Oct 11, 2021
Merged

[New] Support for ESLint v8 #2191

merged 1 commit into from Oct 11, 2021

Conversation

@ota-meshi
Copy link
Contributor

@ota-meshi ota-meshi commented Aug 16, 2021

This PR updates this plugin to support ESLint v8.

Fixes #2211

Copy link
Collaborator

@ljharb ljharb left a comment

I'd prefer to find ways so the existing tests do not change at all, to increase confidence that we're not breaking eslint 2-7 support.

Loading

tests/dep-time-travel.sh Outdated Show resolved Hide resolved
Loading
tests/src/cli.js Outdated Show resolved Hide resolved
Loading
tests/src/cli.js Outdated Show resolved Hide resolved
Loading
tests/src/rules/no-amd.js Outdated Show resolved Hide resolved
Loading
@ota-meshi
Copy link
Contributor Author

@ota-meshi ota-meshi commented Aug 16, 2021

Hmm. I'm not sure what caused this error.

Error while loading rule 'import/no-unused-modules': Failed to load plugin 'import' declared in '.eslintrc': Package subpath './lib/util/glob-util' is not defined by "exports" in /home/runner/work/eslint-plugin-import/eslint-plugin-import/node_modules/eslint/package.json

I think I've fixed it so that this issue doesn't happen.

https://github.com/import-js/eslint-plugin-import/pull/2191/files#diff-7d2908b010ec724c0f3a0d7615cb91f4673d765d52c6ba6607501e23470f2966R21

I have removed the import plugin configuration from ./.eslintrc in my local environment and most tests work fine with npm run tests-only.
I thought that in CI it works fine with linklocal without changing ./.eslintrc, is that wrong?

Loading

@ljharb
Copy link
Collaborator

@ljharb ljharb commented Aug 16, 2021

That message suggests that we're pulling lib/util/glob-util somewhere, and that's no longer a permitted path in eslint.

Loading

@ota-meshi
Copy link
Contributor Author

@ota-meshi ota-meshi commented Aug 16, 2021

lib/util/glob-util appears to be used in the final failover.

https://github.com/import-js/eslint-plugin-import/pull/2191/files#diff-7d2908b010ec724c0f3a0d7615cb91f4673d765d52c6ba6607501e23470f2966R51
image

But the line number in the error seems to point to the previous module (line number 43, new line number is 51).
Is eslint-plugin-import(2.x) in node_modules used for testing?

image

Loading

@ljharb
Copy link
Collaborator

@ljharb ljharb commented Aug 16, 2021

I think the way things work is that the previous version ends up in node_modules, so it's possible the try/catch stuff would need to be landed and published first - but hopefully there's another alternative.

Loading

@ota-meshi
Copy link
Contributor Author

@ota-meshi ota-meshi commented Aug 16, 2021

FileEnumerator#iterateFiles loads the node_modules plugin from eslintrc.
I think we can test it correctly by linking myself eslint-plugin-import to a dependency eslint-plugin-import, is there a way to do this in CI?

Loading

npm i --no-save eslint-plugin-import@'.' -f
echo "Build self"
npm run build
fi
Copy link
Contributor Author

@ota-meshi ota-meshi Aug 17, 2021

Choose a reason for hiding this comment

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

I made it link as a workaround to get the test to work which fails due to eslint-plugin-import in node_modules.

Loading

Copy link
Collaborator

@ljharb ljharb Oct 11, 2021

Choose a reason for hiding this comment

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

Once this is published, will this workaround no longer be needed?

Loading

Copy link
Contributor Author

@ota-meshi ota-meshi Oct 11, 2021

Choose a reason for hiding this comment

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

I expect that the workaround will be unnecessary if we release eslint-plugin-import and change it to use it.
I think that we can also undo .nycrc change.
https://github.com/import-js/eslint-plugin-import/pull/2191/files#r690041296

Loading

tests/src/cli.js Outdated Show resolved Hide resolved
Loading
@ota-meshi
Copy link
Contributor Author

@ota-meshi ota-meshi commented Aug 17, 2021

At this point, I think we've been able to confirm that at least the no-unused-modules.js changes work.
Please let me know if you have any advice on how to change the test cases and CI.

Loading

@@ -71,7 +71,8 @@
"chai": "^4.3.4",
"coveralls": "^3.1.0",
"cross-env": "^4.0.0",
"eslint": "^2 || ^3 || ^4 || ^5 || ^6 || ^7.2.0",
"escope": "^3.6.0",
Copy link
Contributor Author

@ota-meshi ota-meshi Aug 17, 2021

Choose a reason for hiding this comment

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

This change is a workaround to prevent the babel-eslint patch from crashing.

https://github.com/babel/babel-eslint/blob/v8.2.6/lib/patch-eslint-scope.js#L31

Loading

tests/src/cli.js Outdated Show resolved Hide resolved
Loading
@@ -10,6 +10,7 @@
"exclude": [
"coverage",
"test",
"tests"
"tests",
"lib"
Copy link
Collaborator

@ljharb ljharb Aug 17, 2021

Choose a reason for hiding this comment

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

why is this needed?

Loading

Copy link
Contributor Author

@ota-meshi ota-meshi Aug 17, 2021

Choose a reason for hiding this comment

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

This was necessary because it takes a long time to load eslint-plugin-import.

#2191 (comment)

Loading

package.json Outdated Show resolved Hide resolved
Loading
src/rules/no-unused-modules.js Outdated Show resolved Hide resolved
Loading
src/rules/no-unused-modules.js Outdated Show resolved Hide resolved
Loading
tests/dep-time-travel.sh Outdated Show resolved Hide resolved
Loading
tests/src/cli.js Outdated Show resolved Hide resolved
Loading
tests/src/cli.js Outdated Show resolved Hide resolved
Loading
tests/src/cli.js Outdated Show resolved Hide resolved
Loading
tests/src/cli.js Outdated Show resolved Hide resolved
Loading
@ota-meshi
Copy link
Contributor Author

@ota-meshi ota-meshi commented Aug 18, 2021

I think I've done everything I can do with eslint v8 beta right now.
At this time, I understand that this PR will not be merged as is, as CI contains many workarounds.
If you want to merge only the changes in src/rules/no-unused-modules.js and tests/src/cli.js, please let me know as I will open another PR. We have confirmed that these changes work well with the CI of this PR.

Loading

@ljharb
Copy link
Collaborator

@ljharb ljharb commented Aug 19, 2021

The test one, sure - but for the rule, I think it's better if it fails in eslint 8 prior to us supporting it officially.

Loading

@ota-meshi
Copy link
Contributor Author

@ota-meshi ota-meshi commented Aug 25, 2021

I will open a PR that changes tests/src/cli.js.

Loading

@ota-meshi ota-meshi marked this pull request as ready for review Oct 11, 2021
ljharb
ljharb approved these changes Oct 11, 2021
npm i --no-save eslint-plugin-import@'.' -f
echo "Build self"
npm run build
fi
Copy link
Collaborator

@ljharb ljharb Oct 11, 2021

Choose a reason for hiding this comment

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

Once this is published, will this workaround no longer be needed?

Loading

@ljharb ljharb changed the title Support for ESLint v8 [New] Support for ESLint v8 Oct 11, 2021
@ljharb ljharb merged commit 62e2d88 into import-js:main Oct 11, 2021
98 checks passed
Loading
@ljharb ljharb mentioned this pull request Oct 11, 2021
22 tasks
@ota-meshi ota-meshi deleted the eslint-v8 branch Oct 11, 2021
@ykzts ykzts mentioned this pull request Oct 14, 2021
8 tasks
mrtnzlml added a commit to adeira/universe that referenced this issue Oct 14, 2021
mrtnzlml added a commit to adeira/universe that referenced this issue Oct 17, 2021
mrtnzlml added a commit to adeira/universe that referenced this issue Oct 19, 2021
mrtnzlml added a commit to adeira/universe that referenced this issue Oct 26, 2021
mrtnzlml added a commit to adeira/universe that referenced this issue Oct 26, 2021
See:

- https://eslint.org/blog/2021/10/eslint-v8.0.0-released
- https://eslint.org/docs/user-guide/migrating-to-8.0.0

Known issues:

- [x] wait for import-js/eslint-plugin-import#2191 to be released and merged here
- [x] wait for gajus/eslint-plugin-flowtype#496 to be released and merged here
- [x] wait for testing-library/eslint-plugin-testing-library#462 to be released and merged here
- [ ] wait for mysticatea/eslint-plugin-node#294 to be released and merged here
- [ ] ...
mrtnzlml added a commit to adeira/universe that referenced this issue Oct 26, 2021
See:

- https://eslint.org/blog/2021/10/eslint-v8.0.0-released
- https://eslint.org/docs/user-guide/migrating-to-8.0.0

Known issues that need to be fixed first:

- [x] wait for import-js/eslint-plugin-import#2191 to be released and merged here
- [x] wait for gajus/eslint-plugin-flowtype#496 to be released and merged here
- [x] wait for testing-library/eslint-plugin-testing-library#462 to be released and merged here
- [ ] wait for mysticatea/eslint-plugin-node#294 to be released and merged here
- [ ] wait for facebook/react#22248 to be released and merged here
- [ ] release minor/patch version of Adeira Eslint Config before merging this breaking change
- [ ] ...
mrtnzlml added a commit to adeira/universe that referenced this issue Nov 13, 2021
See:

- https://eslint.org/blog/2021/10/eslint-v8.0.0-released
- https://eslint.org/docs/user-guide/migrating-to-8.0.0

Known issues that need to be fixed first:

- [x] wait for import-js/eslint-plugin-import#2191 to be released and merged here
- [x] wait for gajus/eslint-plugin-flowtype#496 to be released and merged here
- [x] wait for testing-library/eslint-plugin-testing-library#462 to be released and merged here
- [ ] wait for mysticatea/eslint-plugin-node#294 to be released and merged here
- [x] wait for facebook/react#22248 to be released and merged here
- [ ] release minor/patch version of Adeira Eslint Config before merging this breaking change
- [ ] ...
mrtnzlml added a commit to adeira/universe that referenced this issue Nov 13, 2021
See:

- https://eslint.org/blog/2021/10/eslint-v8.0.0-released
- https://eslint.org/docs/user-guide/migrating-to-8.0.0

Known issues that need to be fixed first:

- [x] wait for import-js/eslint-plugin-import#2191 to be released and merged here
- [x] wait for gajus/eslint-plugin-flowtype#496 to be released and merged here
- [x] wait for testing-library/eslint-plugin-testing-library#462 to be released and merged here
- [ ] wait for mysticatea/eslint-plugin-node#294 to be released and merged here
- [x] wait for facebook/react#22248 to be released and merged here
- [ ] release minor/patch version of Adeira Eslint Config before merging this breaking change
- [ ] ...
mrtnzlml added a commit to adeira/universe that referenced this issue Nov 23, 2021
See:

- https://eslint.org/blog/2021/10/eslint-v8.0.0-released
- https://eslint.org/docs/user-guide/migrating-to-8.0.0

Known issues that need to be fixed first:

- [x] wait for import-js/eslint-plugin-import#2191 to be released and merged here
- [x] wait for gajus/eslint-plugin-flowtype#496 to be released and merged here
- [x] wait for testing-library/eslint-plugin-testing-library#462 to be released and merged here
- [ ] wait for mysticatea/eslint-plugin-node#294 to be released and merged here
- [x] wait for facebook/react#22248 to be released and merged here
- [ ] release minor/patch version of Adeira Eslint Config before merging this breaking change
- [ ] ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

2 participants