-
-
Notifications
You must be signed in to change notification settings - Fork 66
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!: eslint to esm #88
Conversation
cdd0e0a
to
351b295
Compare
Please don’t do this with anything that another package might want to use; ESM can’t be required, but CJS can be imported, so it’s an inferior format for packages to use - and the only advantage it has is TLA, which is rarely needed outside the entry point. |
Any conversion would also have a commonjs export from the package. |
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 for taking a look at this. I think the proposal lays out some good points, but it too high-level. We use require() in a lot of places inside of ESLint and we would need a plan to address each instance.
} | ||
} | ||
``` | ||
2. dual-mode: compile esm to cjs, publish esm+cjs |
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 think this is the only real option for us at this time.
designs/2022-eslint-to-esm/README.md
Outdated
3. Higher maintenance costs. You need to add build processes, test and publish ESM/CJS code. | ||
4. The CLIs do not support dual-mode. | ||
|
||
Therefore, I would suggest that we eventually go with ESM-Only, but to give users more time to upgrade, as an intermediate state, we could use dual-mode in ESLint V9: |
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 just don’t think we can do this anytime soon. It’s nice to imagine it, but Node.JS ecosystem as a whole is still very reliant on CommonJS.
@nzakas that's great! At that point tho, what’s the benefit? ESM can import CJS, so why choose to ship two copies of eslint when one would suffice? |
While there's no doubt it might be difficult for the ecosystem to migrate in the short term, I haven't seen any real arguments for sticking with CJS in the long term. So the question that remains on migrating is if not now, then, when? |
@dburles when? Possibly never. But certainly not before the majority of the ecosystem is ready to support it - there’s tons of use cases that are impossible without the currently-experimental loaders API, there doesn’t exist a require.resolve for ESM, etc. Migrations need a reason to do it, not not to, and other than TLA, there simply aren’t any. |
Ideally eslint should be the last one to be esm. I had thought eslint v9 - v10(~1 year according to eslint's release history) was enough for most packages. But, yes, I'll update the rfcs to state: "esm-only when the majority of the ecosystem is ready to support it." |
1190427
to
dde3255
Compare
designs/2022-eslint-to-esm/README.md
Outdated
|
||
<!-- Why are we doing this? What use cases does it support? What is the expected | ||
outcome? --> | ||
The Node.js community has been written commonjs for a long time, the same goes for ESLint. But with ESM introduced in ES6 and officially supported by Node.js v12, more and more projects are moving to ESM. Not long ago, we had converted a few repos under the ESLint organization to ESM: |
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.
to put this in context: "more and more" is a pretty big exaggeration; there's one particular author whose outsized package count skews this impression; if they are skipped (or treated as "one" instead of "n"), it's a vanishingly small number.
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 guess you were saying sindre; but he was maintaining a vast number of packages, and cannot be skipped( as being widely used. :)
What do you think of changing to "a growing number of..."? or any other suggestions?
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 think it is very unwise to let a single person’s actions drive change in any ecosystem, regardless of how many packages they publish.
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.
it depends. IMHO, moving to esm-only is dialectical - it's something long-term beneficial, while also suffering short-term pain.
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.
What’s the benefit?
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.
The personal attack and liberal application of thumbs down reacji notwithstanding, it was a real question, asked in good faith.
The first blindingly obvious benefit to using ESM is you can use other ESM packages as dependencies
this is only a benefit if there’s an ESM-only package one needs that isn’t available in CJS - I’m not aware of anything where this is the case, at least for eslint’s purposes.
modules can work in a browser and Deno
The browser doesn’t apply to eslint. Deno compiles TS on the fly; there’s no reason it couldn’t compile CJS on the fly too - but either way, eslint is an npm package, so deno isn’t really relevant - but at least that would be a reasonable benefit if it’s a maintenance burden eslint wants to take on.
HTTP imports provide a much saner
(first of all, “saner” is an ableist term in this context; and using it probably violates eslint’s CoC)
This is an opinion, and one that very much contradicts my experience and security expertise. This brings in all the vulnerabilities and attack surfaces and nondeterminism of DNS, to a server environment where such things are far more dangerous than in the browser. npm is the superior approach - that it’s monetized is irrelevant (except in that it’s necessary in capitalism to monetize something you want to function properly), Also, import maps are not standard yet - “chrome and deno ship them” don’t constitute a standard.
I appreciate the additional context about your packages; it’s good to know who needs to be added to a list of authors to avoid.
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.
But what's the benefits of cjs(except for compat issues)?
ESM is the "standard", and a fully different world than CJS. I don't see the benefit of splitting the community into two. If we must choose one, it's esm.
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 interoperability and compat are one of the biggest benefits that could possible exist.
The problem is that the choice of ESM is what splits the community, because you can import CJS but you can't require ESM. By choosing CJS, you support both communities, and avoid creating friction for users before node's ESM implementation itself, not to mention the overarching ecosystem, is ready.
CJS is a standard too, in node - it's precisely as standard as ESM is there, and both are first-class module systems (ESM is still getting there, but it will eventually) in node for the foreseeable future.
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.
the interoperability and compat issue will not be a strong need if the community moving to esm. Then we will have a more united community. :)
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 don't believe that will ever happen - but, even if it would, then the time to do this migration is after that - not merely because some dream of it.
CJS will be around forever.
``` | ||
2. dual-mode: compile esm to cjs, publish esm+cjs | ||
```js | ||
"type": "module", |
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.
"type": "module", |
i've made this point elsewhere, but there's no benefit to type module - it just makes .js
files be ESM. ESM files should be .mjs
, and then this aesthetics-only escape hatch won't be needed.
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.
we had been using "type": "module"
in other eslint repos. I'd be supportive to change all others if want .mjs
- for consistency. cc @nzakas @mdjermanovic
Node.js ESM has a limitation: ESM cannot be required. Approach 2 provides a good compatibility for existing projects that rely on ESLint without requiring users to adopt ESM as well. In previous repos, dual-mode was mostly used (except for '@eslint/create-config'). | ||
|
||
However, dual-mode also has a few disadvantages: | ||
1. dependencies. Some ESLint's dependencies have been (or will be) esm-only, which means we won't be able to upgrade them if we continue to support CJS. For new dependencies, we cannot use them if they are esm-only. |
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 dep going ESM-only is user-hostile - not nearly as bad as one that violates semver, but in a similar bucket. If eslint is depending on packages that have done this, and there truly aren't alternatives in the ecosystem, I'll be happy to create and maintain them for as long as eslint needs them.
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.
It would be good if all the dependents go to esm, but it will take a very very long time. 😄
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.
That will be the case whether eslint goes ESM-only or not - it can’t be forcibly accelerated.
designs/2022-eslint-to-esm/README.md
Outdated
## Toolings esm supports | ||
|
||
* mocha ✅ | ||
* proxyquire ❌ |
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.
until the loaders API is no longer experimental, mocking of ESM deps isn't available.
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.
we can use the experimental loader, as it's only used in the tests.
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.
ah, maybe i misunderstood. consumers who rely on the ability to mock eslint are who i'm concerned about here.
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.
the list was the toolings that eslint was relying on. will update to clarify, and add the mocking eslint issue in FAQ.
designs/2022-eslint-to-esm/README.md
Outdated
+ var { ESLint } = await import("eslint"); | ||
``` | ||
|
||
c. compile ESLint to CJS. |
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.
it's simply not tenable or safe to transpile code one didn't author, so i'd suggest eslint not recommend this at all.
* nyc ❌ | ||
* eslint-plugin-node ❗️(Not fully supported, but we could use a fork: `eslint-plugin-n`) | ||
|
||
## Implementations |
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.
Another task to be aware of. Any dynamic importing with require()
that might be happening inside functions (i.e. not at the top of the file) will need to be changed to await import()
and that means changing the wrapping function to be async as well as any functions that call it (could be a lot of functions becoming async).
I encountered this when converting another linter to esm: https://github.com/ember-template-lint/ember-template-lint/pull/2262/files#diff-89b11f27e82cbe2d696dc943a52c076842b66a9a9abcc1fdb5ee1287968ffde5
## Implementations | ||
1. ESLint v9 | ||
* package.json + `"type": "module"` | ||
* package.json 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.
Worth indicating which of these ESLint has already implemented (such as exports
).
--> | ||
1. Compatibility issues, see below. | ||
|
||
## Backwards Compatibility Analysis |
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.
Would be good to call out the impact to third-party plugin developers. Can plugins write rules/configs in either CJS/ESM? Can plugins be entirely ESM? What about rule tests? Currently they use: const RuleTester = require('eslint').RuleTester;
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.
The problem is that currently plugins can not be written in esm! This is the reason that I'm reading this thread. It should be no problem to write plugins in cjs, once eslint is switched to esm.
We are in the process of switching over all our packages (over a hundred, both public and internal) to esm. The reason we are doing this is: (1) it's the only way to effectively tree-shake (2) other packages we rely on started switching to esm
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 will be a non-issue going forward. With the new config system it doesn’t matter how you write your plugins because you’re in charge of loading them.
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.
@simlu fwiw, the better way to effectively tree-shake is to granularly import only what you need - it does a better job than any treeshaking possibly can :-)
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.
@ljharb LOL, better even, custom rewrite everything so it's the minimal code you need 😜 /sarcasmend
Sorry I should have been more concise. I was talking about ncc. So not traditional tree shaking and what you import really doesn't matter much (but yes we do that already)
Co-authored-by: Jordan Harband <ljharb@gmail.com>
17e6bcb
to
2650297
Compare
2650297
to
6c88de8
Compare
6c88de8
to
233de88
Compare
Given that we are planning a complete rewrite of ESLint, I think it probably makes sense not to convert the existing eslint repo into ESM and instead start fresh with a new repo that is written in ESM from the start. Thoughts? |
either is fine to me. the other side I can see is the rewriting can take a very long time(maybe a few years), it means we'll have to maintain the two repos during this time. It won't hurt to do the conversion, and it also helps to improve our development experience in maintaining the old repo. |
I think the conversion will also take a very long time and will be more complicated than we think, that's why I think it's a better idea to focus on the "new" ESLint rather than spending time on conversion. @mdjermanovic what do you think? |
Converting existing ESLint to ESM could be seen as one incremental step to a larger rewrite. Some of the resulting ESM code could be reusable in the rewrite, or it could enable us to more easily swap out individual pieces of the codebase during an incremental rewrite. I'm not 100% attached to it, but I do think it's important to de-risk large rewrites, many of which fail because they went for an all-or-nothing approach (i.e. years spent attempting to rewrite everything at once without any usable output) instead of an incremental approach (i.e. swapping out smaller pieces one-by-one while keeping things functional), so ESM sooner could still be a good idea. |
I also think converting the existing eslint/eslint repo into ESM will take a lot of time and effort, and I agree with @nzakas that, given that we are planning a complete rewrite in a separate repo, we should keep the existing repo in the CJS format. Long-term, we all agree that ESLint repo should be ESM, but we'll get there with the "new" ESLint. Therefore, investing a lot of time in converting the existing repo doesn't seem justified to me. |
well, looks like we had a consensus not converting, so closing. :) |
rendered: https://github.com/aladdin-add/rfcs/tree/2022-eslint-to-esm/designs/2022-eslint-to-esm
Summary
Convert the ESLint codebase to ESM and publish the ESM version.
Related Issues