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: ES Module Compatibility #43

Merged
merged 9 commits into from Dec 2, 2019

Conversation

@evanplaice
Copy link
Contributor

@evanplaice evanplaice commented Oct 6, 2019

On Hold: This RFC is on hold until Node unflags ES modules

Summary

JS-based ESLint configurations break when used in an ESM-based package. This RFC documents the specifics outlining the cause of the break as well as a forward-compatible fix.

Related Issues

Copy link
Member

@mysticatea mysticatea left a comment

Thank you for your contribution!

I have some suggestions.

And, ESLint doesn't support experimental stuff basically. I believe that we must wait for the stable release of ES modules in Node in order to accept this RFC.

designs/2019-esm-compatibilty/README.md Outdated Show resolved Hide resolved
designs/2019-esm-compatibilty/README.md Outdated Show resolved Hide resolved
designs/2019-esm-compatibilty/README.md Show resolved Hide resolved
designs/2019-esm-compatibilty/README.md Outdated Show resolved Hide resolved
*/lib/cli-engine/config-array-factory.js*

```diff
function loadConfigFile(filePath) {

This comment has been minimized.

@mysticatea

mysticatea Oct 6, 2019
Member

This function doesn't affect how ESLint finds config files. We should update the finding logic.

This comment has been minimized.

@evanplaice

evanplaice Oct 10, 2019
Author Contributor

I could use some help w/ this, I'm not very familiar w/ the core internals other than what I've been able to find so far.

This comment has been minimized.

@mysticatea

mysticatea Oct 11, 2019
Member

Actually, RFCs don't need to include implementation :)

For a reference, this array is the order to find config files. ESLint checks files in the array one by one then adopts the first found one.

Therefore the current priority is:

  1. .eslintrc.js
  2. .eslintrc.yaml
  3. .eslintrc.yml
  4. .eslintrc.json
  5. .eslintrc
  6. package.json

This RFC should include how it changes this priority because it's a user-facing change.

This comment has been minimized.

@mysticatea

mysticatea Oct 11, 2019
Member

And the priority is documented in the "Configuration File Formats" section. The "Documentation" section in this RFC should include it.

This comment has been minimized.

@evanplaice

evanplaice Oct 15, 2019
Author Contributor

The required change would put .eslintrc.cjs at priority 1. In short, it's an 'escape hatch' that will primarily impact users who use "type": "module" and .eslintrc style configs.

I'll see if I can clarify the descriptions and reserve implementation specifics for the related PR.

This comment has been minimized.

@evanplaice

evanplaice Oct 15, 2019
Author Contributor

In this case it'll become:

  1. .eslintrc.cjs
  2. .eslintrc.js
  3. .eslintrc.yaml
  4. .eslintrc.yml
  5. .eslintrc.json
  6. .eslintrc
  7. package.json

The implementation specifics have been removed in the RFC, this change will be added to the PR instead.

This comment has been minimized.

@mysticatea

mysticatea Oct 18, 2019
Member

Would you include the priority change in this RFC? And would you add about we need to update the "Configuration File Formats" section to the Documentation section of this RFC?

This comment has been minimized.

@evanplaice

evanplaice Oct 25, 2019
Author Contributor

Done. See Revision 5.

designs/2019-esm-compatibilty/README.md Outdated Show resolved Hide resolved
@evanplaice
Copy link
Contributor Author

@evanplaice evanplaice commented Oct 10, 2019

And, ESLint doesn't support experimental stuff basically. I believe that we must wait for the stable release of ES modules in Node in order to accept this RFC.

I assumed as much. If all goes well, ESM support should roll out later this month. Can this RFC be tagged as 'onhold' until then?

- fix RFC ref
- rewrite summary, less fluff more substance
- use the correct config filenames
- remove alternative 3 (NA)
- add a ref to the issue that spawned this RFC
@jkrems
Copy link

@jkrems jkrems commented Oct 14, 2019

And, ESLint doesn't support experimental stuff basically. I believe that we must wait for the stable release of ES modules in Node in order to accept this RFC.

Please note that the error already had landed in a stable version of node (not behind a flag). We rolled that change back because of issues in ESLint and babel. So if not for eslint, it would still be in a stable version. We're currently counting on ESLint to add the necessary fixes so we can ship the feature again.

@mysticatea
Copy link
Member

@mysticatea mysticatea commented Oct 15, 2019

@jkrems That has been reverted in 12.11.1.

@jkrems
Copy link

@jkrems jkrems commented Oct 15, 2019

@mysticatea
Copy link
Member

@mysticatea mysticatea commented Oct 15, 2019

Oh, sorry, I had misunderstood you.

I had thought that the change has been reverted because it's a breaking change and to wait for ES modules gets stable. But in fact, the change has been reverted to wait for tools are fixed?

@jkrems
Copy link

@jkrems jkrems commented Oct 15, 2019

I had thought that the change has been reverted because it's a breaking change and to wait for ES modules gets stable. But in fact, the change has been reverted to wait for tools are fixed?

The change is in that gray area of "is this breaking?" that comes with new features that people have tried polyfilling already. People used polyfills (e.g. via the esm package or babel transforms) to already use type: module today but those aren't actually behaving like the real thing. So when we tried to roll out the first piece of the real thing, those people ran into issues (like the eslint config files). The rollback was mostly meant to give people a second chance to prepare. But .cjs is here to stay or rather already de-facto exists[1] and we are working from the assumption that it will be supported by tools.

[1] "Already de-facto exists" because using any random file extension for CommonJS always worked... :D

@platinumazure
Copy link
Member

@platinumazure platinumazure commented Oct 15, 2019

@jkrems Could you please explain what exactly would need to change in ESLint to support this (under the assumption that Node is actually trying to release this now)? If that's already covered somewhere, please feel free to link. Thanks!

@jkrems
Copy link

@jkrems jkrems commented Oct 15, 2019

@platinumazure I think this proposal ("support .eslintrc.cjs") would be sufficient to unblock users. Right now they don't have a way to provide a CommonJS config to eslint from within a type:module package because the only extension eslint recognizes (.js) isn't valid for CommonJS in that context. We're softening invalid CommonJS requires to a warning (nodejs/node#29909) to provide some visibility before bringing back the exception. The warning is nudging users towards .cjs if they have the choice.

There is the separate question if eslint wants to recognize config files written as modules but I don't think that has to be addressed with the same urgency.

Copy link
Member

@platinumazure platinumazure left a comment

I've identified only 1-2 real issues/questions that need to be addressed here.

That said, I also put on my editor hat and made a number of minor suggestions which you can accept or ignore as you please. 😄

Let me know if you need clarification on which changes are "blocking" vs "non-blocking" in my view. Thanks!

designs/2019-esm-compatibilty/README.md Outdated Show resolved Hide resolved
designs/2019-esm-compatibilty/README.md Outdated Show resolved Hide resolved
designs/2019-esm-compatibilty/README.md Outdated Show resolved Hide resolved
designs/2019-esm-compatibilty/README.md Outdated Show resolved Hide resolved
designs/2019-esm-compatibilty/README.md Outdated Show resolved Hide resolved
designs/2019-esm-compatibilty/README.md Outdated Show resolved Hide resolved
designs/2019-esm-compatibilty/README.md Outdated Show resolved Hide resolved
designs/2019-esm-compatibilty/README.md Outdated Show resolved Hide resolved
designs/2019-esm-compatibilty/README.md Outdated Show resolved Hide resolved
designs/2019-esm-compatibilty/README.md Outdated Show resolved Hide resolved
@platinumazure
Copy link
Member

@platinumazure platinumazure commented Oct 15, 2019

Thanks @jkrems for the explanation, it was really helpful.

I've reviewed this RFC based on my current understanding of the proposal and background. It is possible I am misunderstanding something-- if so, I greatly appreciate your patience as we work through this process together. (I also made some minor suggestions which I think might improve readability.)

@jkrems
Copy link

@jkrems jkrems commented Oct 15, 2019

Thanks for taking your time to help review this proposal, it's much appreciated. :) I can definitely understand reservations about jumping the gun on experimental features. This situation would've been less painful and we could've saved a rollback if people didn't start using type:module before node started shipping it (so 👍 to being a bit conservative).

Happy to help work this out! I think there were also people from the nodejs/modules side eager to help with implementation once the RFC has stabilized.

@evanplaice
Copy link
Contributor Author

@evanplaice evanplaice commented Oct 15, 2019

There is the separate question if eslint wants to recognize config files written as modules but I don't think that has to be addressed with the same urgency.

Just to be clear. This RFC does not address defining the configuration as an ES module. This only addresses making existing ESLint configurations compatible with the new module format.

I'll add a mention in the RFC to address this.

@platinumazure Thank you for reviewing this. Feel free to nit as much as you feel necessary. I'll try my best to dilute the minutiae as much as possible so it's palatable to a more general audience.

- fix nits and spelling errors
- rewite the intro again for clarity
- change future to present tense
- rewrite 'Motivation' to be more concise
- remove implementation specifics
- add FAQ covering ESM-based configs
- add reference to the implementation PR
@evanplaice evanplaice force-pushed the evanplaice:2019-esm-compatibility branch from 421aa8e to 27f69f4 Oct 15, 2019
- fix more nits
- correct the name/link to the related issue
- fix grammatical mistake
Copy link
Member

@platinumazure platinumazure left a comment

Looks good from my perspective. Thanks!

Mention configuration loader priority
6. .eslintrc
7. package.json

`.cjs` is placed higher than `.js` because -- unlike the latter -- `.cjs` does not allow for ambiguity. A `.cjs` file is always interpreted as CommonJS regardless of context.

This comment has been minimized.

@ljharb

ljharb Oct 25, 2019

to be clear, .js is always and by default unambiguously CJS, unless in a package.json boundary with "type": "module", when it's unambiguously ESM.

This comment has been minimized.

@jkrems

jkrems Oct 25, 2019

to be clear, .js is always and by default unambiguously CJS, unless in a package.json boundary with "type": "module", when it's unambiguously ESM.

I think that's adding an assumption to the sentence it didn't contain before: That we know anything but "this one file has the extension .cjs". With only that information, it is ambiguous in exactly the way you describe. :)

This comment has been minimized.

@jkrems

jkrems Oct 25, 2019

(It may be confusing that in other contexts we have used "ambiguous" to describe the problem of files being ambiguous even given all metadata we could possibly look at.)

This comment has been minimized.

@evanplaice

evanplaice Oct 25, 2019
Author Contributor

Removed explanation in Revision 6

This comment has been minimized.

@ljharb

ljharb Oct 25, 2019

@jkrems a node tool always has the additional default context "based on node's resolution algorithm" :-)

akash1810 added a commit to guardian/cdk that referenced this pull request Mar 31, 2021
Publishing `@guardian/cdk` in ESM format is tricky as additional tooling required by consumers such as [ts-node](TypeStrong/ts-node#1007) (see also [this](TypeStrong/ts-node#935)), [ESLint](eslint/rfcs#43) and [Jest](https://jestjs.io/docs/ecmascript-modules) have varying support.

It is easiest to stay on CommonJS.

We'll have to stay on v7.0.1 of `read-pkg-up` to achieve this; v8.0.0 doesn't bring any new features or fixes, so this is safe.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants