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

feat!: Start using enhanced-resolve to improve ts support #139

Merged
merged 15 commits into from
Jan 9, 2024

Conversation

scagood
Copy link

@scagood scagood commented Nov 9, 2023

This PR is going to aim to:

I am going to be making the following changes:

  • Change to using enhanced-resolve
  • Check for and resolve "import type" in import-target
  • Attempt to import and check typescript "paths"
  • Allow for passing ts extensions into the resolve function

@scagood scagood force-pushed the resolve branch 2 times, most recently from 3ccd883 to 8e0dcb7 Compare November 10, 2023 10:25
scagood

This comment was marked as outdated.

@scagood scagood added this to the v17 milestone Nov 22, 2023
@scagood scagood force-pushed the resolve branch 2 times, most recently from 9510597 to aa800e5 Compare November 25, 2023 16:57
Comment on lines 317 to 343
{
// name: '.js has a higher priority than .json'
filename: fixture("test.js"),
code: "import './multi'",
output: null,
output: "import './multi.js'",
options: ["always"],
errors: [
{ messageId: "requireExt", data: { ext: ".cjs or .mjs" } },
],
errors: [{ messageId: "requireExt", data: { ext: ".js" } }],
},
{
filename: fixture("test.js"),
code: "import './multi.js'",
output: null,
options: ["never"],
errors: [{ messageId: "forbidExt", data: { ext: ".js" } }],
},
{
filename: fixture("test.js"),
code: "import './multi.cjs'",
code: "import './multi.json'",
output: null,
options: ["never"],
errors: [{ messageId: "forbidExt", data: { ext: ".cjs" } }],
errors: [{ messageId: "forbidExt", data: { ext: ".json" } }],
},
Copy link
Author

Choose a reason for hiding this comment

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

These multi tests did not make sense, as you cant import .cjs or .mjs files without a file extension. (that I know of / found)

I have thus swapped the test to use .js and .json.
This means that we can use the correct import priority as a fixer.

Choose a reason for hiding this comment

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

I'm wondering if suggestion would be more appropriate: https://eslint.org/docs/latest/extend/custom-rules#providing-suggestions

generally, it's a best practice to not change the runtime behavior in fixers.

Copy link
Author

Choose a reason for hiding this comment

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

🤔 We can try to do that!

lib/rules/file-extension-in-import.js Show resolved Hide resolved
Comment on lines +146 to +143
if (isBuiltin(this.name)) {
return "node"
}

if (/^(@[\w~-][\w.~-]*\/)?[\w~-][\w.~-]*/.test(this.name)) {
return "npm"
}
Copy link
Author

Choose a reason for hiding this comment

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

I wonder what to do with these when people use the @organisation/package in path aliases.

@scagood scagood changed the title [WIP] type only module marked as missing feat!: Start using enhanced-resolve to improve ts support Nov 26, 2023
@scagood scagood requested a review from a team November 26, 2023 16:30
@scagood scagood marked this pull request as ready for review November 26, 2023 16:30
@scagood

This comment was marked as outdated.

@aladdin-add
Copy link

I was waiting for eslint v9 to be released, and to see if there are some other breaking changes needed.

It's fine to put it in one PR. I'll review it later, sorry for the delay. :😏

@scagood
Copy link
Author

scagood commented Dec 15, 2023

Ah cool! That makes complete sense 😁

I was just thinking about this comment #147 (comment). More specifically about trying to implement the settings.import/resolver api in here as well.

You're 100% right, I can hold my horses though, stability is far more important!

lib/rules/file-extension-in-import.js Show resolved Hide resolved
lib/util/import-target.js Outdated Show resolved Hide resolved
lib/util/import-target.js Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
Comment on lines 317 to 343
{
// name: '.js has a higher priority than .json'
filename: fixture("test.js"),
code: "import './multi'",
output: null,
output: "import './multi.js'",
options: ["always"],
errors: [
{ messageId: "requireExt", data: { ext: ".cjs or .mjs" } },
],
errors: [{ messageId: "requireExt", data: { ext: ".js" } }],
},
{
filename: fixture("test.js"),
code: "import './multi.js'",
output: null,
options: ["never"],
errors: [{ messageId: "forbidExt", data: { ext: ".js" } }],
},
{
filename: fixture("test.js"),
code: "import './multi.cjs'",
code: "import './multi.json'",
output: null,
options: ["never"],
errors: [{ messageId: "forbidExt", data: { ext: ".cjs" } }],
errors: [{ messageId: "forbidExt", data: { ext: ".json" } }],
},

Choose a reason for hiding this comment

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

I'm wondering if suggestion would be more appropriate: https://eslint.org/docs/latest/extend/custom-rules#providing-suggestions

generally, it's a best practice to not change the runtime behavior in fixers.

tests/lib/rules/no-missing-require.js Outdated Show resolved Hide resolved
Copy link

@SlyryD SlyryD left a comment

Choose a reason for hiding this comment

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

Looking forward to the improved support!

Copy link

@aladdin-add aladdin-add left a comment

Choose a reason for hiding this comment

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

awesome work, thanks for doing this!

The changes look good; let's merge them when we are preparing for the v17 release in our development.

@aladdin-add aladdin-add mentioned this pull request Jan 3, 2024
@aladdin-add aladdin-add marked this pull request as draft January 3, 2024 03:14
@aladdin-add aladdin-add marked this pull request as ready for review January 8, 2024 09:44
@aladdin-add
Copy link

looks we have a conflict now. @scagood

@scagood
Copy link
Author

scagood commented Jan 8, 2024

mmmm, its not a simple merge. I will rebase later 😂

@aladdin-add aladdin-add merged commit dc9f473 into eslint-community:master Jan 9, 2024
17 checks passed
@scagood scagood deleted the resolve branch January 9, 2024 09:42
@scagood scagood linked an issue Feb 14, 2024 that may be closed by this pull request
1 task
kevinoid pushed a commit to kevinoid/eslint-config-kevinoid that referenced this pull request Apr 10, 2024
Bumps [eslint-plugin-n](https://github.com/eslint-community/eslint-plugin-n) from 16.6.2 to 17.1.0.
- [Release notes](https://github.com/eslint-community/eslint-plugin-n/releases)
- [Changelog](https://github.com/eslint-community/eslint-plugin-n/blob/master/CHANGELOG.md)
- [Commits](eslint-community/eslint-plugin-n@16.6.2...v17.1.0)

---
updated-dependencies:
- dependency-name: eslint-plugin-n
  dependency-type: direct:development
  update-type: version-update:semver-major
...

BREAKING CHANGES:
- drop eslint v7 & node.js < 18 (eslint-community/eslint-plugin-n#161) (eslint-community/eslint-plugin-n@41ceed7)
- Start using enhanced-resolve to improve ts support (eslint-community/eslint-plugin-n#139) (eslint-community/eslint-plugin-n@dc9f473)
- rename rule shebang => hashbang, deprecate rule shebang (eslint-community/eslint-plugin-n#198)

Signed-off-by: dependabot[bot] <support@github.com>
kevinoid pushed a commit to kevinoid/eslint-config-kevinoid that referenced this pull request Apr 10, 2024
Bumps [eslint-plugin-n](https://github.com/eslint-community/eslint-plugin-n) from 16.6.2 to 17.1.0.
- [Release notes](https://github.com/eslint-community/eslint-plugin-n/releases)
- [Changelog](https://github.com/eslint-community/eslint-plugin-n/blob/master/CHANGELOG.md)
- [Commits](eslint-community/eslint-plugin-n@16.6.2...v17.1.0)

---
updated-dependencies:
- dependency-name: eslint-plugin-n
  dependency-type: direct:development
  update-type: version-update:semver-major
...

BREAKING CHANGES:
- drop eslint v7 & node.js < 18 (eslint-community/eslint-plugin-n#161) (eslint-community/eslint-plugin-n@41ceed7)
- Start using enhanced-resolve to improve ts support (eslint-community/eslint-plugin-n#139) (eslint-community/eslint-plugin-n@dc9f473)
- rename rule shebang => hashbang, deprecate rule shebang (eslint-community/eslint-plugin-n#198)

Signed-off-by: dependabot[bot] <support@github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment