-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Update: support .eslintrc.cjs (refs eslint/rfcs#43) #12321
Conversation
Thanks for the PR. We ask that changes to core go through the RFC process outlined here. Do you mind doing that? Incidentally, I don't think we should consider this a bug fix. It would be an enhancement because it would be adding support for an experimental Node feature. |
Yeah, this is not a bug fix. I think too early to merge this feature because ES modules on Node.js is still experimental and it may change due to the large impact to JS ecosystem. |
Speaking from experience, any conversation discussing modules + node has a tendency to fall down the rabbit hole. I posted this PR so there's an actual concrete example to look at. I don't expect this PR to land quickly, if at all.
Not at all, it'll take me a bit to gather and record my thoughts Of course. The initial issue said 'bug' so I followed that workflow. I'll fix it once I get a feel for the process. As far as the time schedule goes -- if nothing major has or will change between now and then -- modules will be officially released (ie unflagged) on 20 October. There's still time to discuss, bike shed, shave a few yaks, and decide the best approach for ESLint specifically. |
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.
This looks good to me at first glance (so far).
I'm a bit surprised there aren't any tests. I guess I would like to see if we could add a new test for the CJS case at least, to avoid regressions.
No need to make any immediate changes, as the RFC is still being discussed. This is just a preliminary review from me. 😄
Update: As mentioned in this issue it looks like the schedule for unflagging ES modules is going to be pushed a bit. |
Update: The unflagging PR (nodejs/node#29866) has now landed, an unflagged version of module support in node should be part of the next 13.x release next week. |
Hi. Thank you for working on this. RFC43 has been merged in the last week. |
In ES module packages w/ "type": "module" defined treat all .js files as ES modules. CommonJS files contained in an ES module package should use the .cjs extension.
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.
Do you mind adding some tests? You can find relevant tests in https://github.com/eslint/eslint/blob/master/tests/lib/cli-engine/config-array-factory.js.
I added test cases to:
And added the test:
As well as the fixture:
Is that sufficient? AFAIK, these are the tests that cover config loading. The rest of the tests appear to dig into specific JavaScript-configuration behaviors which should be no different between |
The Should I squash + rebase after it's fixed or just leave it? |
|
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.
This LGTM. Thank you for working on this!
@kaicataldo I'm happy to help. Big thanks to you and everybody else who was involved in this for your patience. |
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.
Looks good, thanks for contributing!
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. Thank you!
Thanks for contributing to ESLint! |
In ES module packages w/ "type": "module" defined treat all .js files as ES modules. Since
eslint
is expecting the config in CommonJS format, it needs to be named w/ a.cjs
extension.What is the purpose of this pull request? (put an "X" next to item)
[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[x] Add something to the core
[ ] Other, please explain:
For details about the issue, see #12319.
For details about the resolution, see RFC #43
What changes did you make? (Give an overview)
Map the
.cjs
file extension for loading CommonJS configurations (ie.eslintrc
) in ES module based packages.Is there anything you'd like reviewers to focus on?
After searching through the source, I didn't see any tests that cover this code. Is there an existing test I need to add/update to cover this functionality?
Also, what section of the docs should ES module usage be documented?