-
-
Notifications
You must be signed in to change notification settings - Fork 30
feat: support named exports in ESM/TS #449
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
Conversation
This adds support for named exports in ESM (whether it be TS or not). For example: ```ts export const rule: RuleModule = { create: () => { ... }; }; ``` Also, exported symbols: ```ts const rule = { ... }; export {rule}; ``` While ESLint plugins are still usually CJS at time of writing this, many are written as ESM sources, and an increasing number are using named exports.
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.
Can you speak to the possibility of this causing additional false positives as I mentioned in #375 (comment)? Do you anticipate any? It may be worth testing on a few top ESLint plugins for a change like this. Is there anything else we should do to mitigate this from detecting named exports that aren't ESLint rules?
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.
According to the eslint-remote-tester
's result, no false positives were found. We can start with this implementation and improve it if we receive feedback. thoughts?
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.
A very primitive idea is allowing configuring the esquery selectors for the possible rules:
{
settings: {
// to allow "export const rule = ..."
"eslint-plugin": {"rule-selectors": ["ExportNamedDeclaration[declaration.type="VariableDeclaration"][declaration.declarations.0.id.name="rule"]"]}
}
}
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.
@aladdin-add is eslint-remote-tester
actually showing us new lint violations (which could be false positives), or just crashes? It says Results: No errors
but I'd be kind of surprised if there really are no lint violations in any of those repositories. We may also want to add more ESM plugins to the list it tests against.
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.
@43081j can you manually test this on a few larger/top ESM plugins? I don't think eslint-remote-tester, which runs during CI, detects false positives for us, only crashes.
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.
i'll have a look into it 👍
Thanks for the quick review @bmish , I'll sort the changes tomorrow 👍 |
FYI i have at least resolved the two changes not sure what you want to do about the testing. since it seems like @aladdin-add already ran the remote tester successfully |
@bmish i ran it against:
and all seemed well |
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!
This adds support for named exports in ESM (whether it be TS or not).
For example:
Also, exported symbols:
While ESLint plugins are still usually CJS at time of writing this, many are written as ESM sources, and an increasing number are using named exports.
Reviewer notes
You'll want to hide whitespace changes when reviewing this, as prettier reformatted a bunch of it thanks to changing the level of nesting/chaining.
Fixes #375