-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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: drop support for jiti v1.21 #18996
Conversation
✅ Deploy Preview for docs-eslint canceled.
|
/** | ||
* Used to import the jiti dependency. This method is exposed internally for testing purposes. | ||
* @returns {Promise<Record<string, unknown>>} A promise that fulfills with a module object | ||
* or rejects with an error if jiti is not found. | ||
*/ | ||
static loadJiti() { | ||
return import("jiti"); | ||
} | ||
|
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.
Any better way to make this function mockable?
👍 It sounds reasonable to me that we don't maintain compatibility with the previous major line, especially since this is still an experimental feature. |
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. Awaiting 2nd review.
lib/config/config-loader.js
Outdated
return config?.default ?? config; | ||
return config.default ?? config; |
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.
Without the ?
, a config like this would result in TypeError: Cannot read properties of undefined (reading 'default')
:
// eslint.config.cts
module.exports = undefined;
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.
is that a valid config/usecase?
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.
Not really, but currently it's caught by validations rather than crashing so the error message is more descriptive:
Oops! Something went wrong! :(
ESLint: 9.12.0
TypeError: Config (unnamed): Unexpected undefined config at user-defined index 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.
Thanks! I reverted the change and added unit tests for this case in e5d3a9b.
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, thanks!
Prerequisites checklist
What is the purpose of this pull request? (put an "X" next to an item)
[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[X] Other, please explain:
What changes did you make? (Give an overview)
Support for jiti v2 was added in PR #18954, but jiti v1.21 is still supported. This can lead to a confusing error message when a project with jiti v1.21 tries to use a TypeScript ESLint config with top-level await:
The correct message should state:
StackBlitz Repro
This is not a regression in #18954. We were getting the same error message previously with jiti v1 where TLA is not supported. Now that jiti v2 is available it seems no longer necessary to maintain compatibility with jiti v1. TypeScript config files are still an experimental feature, so this change will not be a breaking one.
I've also added unit tests to check the error behavior when an outdated jiti versions is installed, or when jiti is not installed.
Is there anything you'd like reviewers to focus on?