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

feat!: eslint to esm #88

Closed
wants to merge 11 commits into from
244 changes: 244 additions & 0 deletions designs/2022-eslint-to-esm/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,244 @@
- Repo: eslint/eslint
- Start Date: (2022-02-28)
- RFC PR: https://github.com/eslint/rfcs/pull/88
- Authors: aladdin-add(weiran.zsd@outlook.com)

# convert eslint codebase to esm

## Summary

<!-- One-paragraph explanation of the feature. -->
Convert the ESLint codebase to ESM and publish the ESM version.
## Motivation

<!-- Why are we doing this? What use cases does it support? What is the expected
outcome? -->
The Node.js community has been writing 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:
* eslint/eslintrc
* eslint/espree
* eslint/eslint-scope
* eslint/eslint-visitor-keys
* eslint/create-config

## Detailed Design

<!--
This is the bulk of the RFC.

Explain the design with enough detail that someone familiar with ESLint
can implement it by reading this document. Please get into specifics
of your approach, corner cases, and examples of how the change will be
used. Be sure to define any new terms in this section.
-->
There are two main ways to achieve this:
1. esm-only: write esm, publish esm
```js
{
"type": "module",
"exports": {
"./package.json": "./package.json",
".": "./lib/api.js",
"./use-at-your-own-risk": "./lib/unsupported-api.js"
}
}
```
2. dual-mode: compile esm to cjs, publish esm+cjs
Copy link
Member

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.

```js
"type": "module",
Copy link
Sponsor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"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.

Copy link
Member Author

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

"exports": {
"./package.json": "./package.json",
".": {
"import": "./lib/api.js",
"require": "./dist/api.cjs",
"default": "./dist/api.cjs"
},
"./use-at-your-own-risk": {
"import": "./lib/unsupported-api.js",
"require": "./dist/unsupported-api.cjs",
}
}
```

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 downsides:
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.
Copy link
Sponsor

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.

Copy link
Member Author

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. 😄

Copy link
Sponsor

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.

2. Slower installation & larger volume usage, even though most users only use ESM, they still need to download CJS modules.
3. Higher maintenance costs. You need to add a build process, test and publish ESM/CJS code.

In the (very) long run, 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:

* eslint v9:
- ESLint CLI: esm-only(`./bin/eslint.js` will be esm, and won't be compiled to cjs.)
- Node.js API: dual mode
* eslint 1x (at the point when the majority of the ecosystem is ready to support it, likely to be v10/v11/...):
- ESLint CLI: esm-only
- Node.js API: esm-only

## Toolings esm supports(ESLint's dependencies)

* mocha ✅
* proxyquire ❌ (not supported, an alternative is [esmock](https://github.com/iambumblehead/esmock), which provide a similar API)
* nyc ❌ (not supported, an alternative is [c8](https://github.com/bcoe/c8) - same author with nyc)
* eslint-plugin-node ❗️(Not fully supported, but we could use a fork: `eslint-plugin-n`)

## Implementations
aladdin-add marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Sponsor Member

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

aladdin-add marked this conversation as resolved.
Show resolved Hide resolved
aladdin-add marked this conversation as resolved.
Show resolved Hide resolved

0. esm toolings for better esm supports
* eslint-plugin-node => eslint-plugin-n
* nyc => c8

1. ESLint v9
* ~~package.json + `"type": "module"`~~renaming `*.js` => `*.mjs`
* package.json exports
Copy link
Sponsor Member

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).

* package.json engines >= Node.js v12 (has been shipped in ESLint v8.0)
* remove `"use strict"`
* convert `"./foo"` to `"./foo/index.js"`
* convert `"./foo"` to `"./foo.js"`
* top-level `require/module.exports` => `import/export`
* non-top-level `require/module.exports`
a). convert to `import("mod")`
b). or use `createRequire`
```js
import {createRequire} from "module";
const require = createRequire(import.meta.url);
````

* convert `__dirname/__filename` => `import.meta.url`.
[#what-do-i-use-instead-of-__dirname-and-__filename](https:\/\/gist.github.com/sindresorhus/a39789f98801d908bbc7ff3ecc99d99c#what-do-i-use-instead-of-__dirname-and-__filename)
* esm toolings(`nyc` -> `c8`, `proxyquire` -> `esmock`)

* cjs supports(build `dist/api.cjs` and `dist/use-at-your-own-risk.cjs` using rollup)
* update docs to use esm

2. ESLint 1x
* remove package.json exports.cjs (use esm-only)
* remove build process
* upgrade blocked deps.

## Documentation

<!--
How will this RFC be documented? Does it need a formal announcement
on the ESLint blog to explain the motivation?
-->
Yes. When ESLint v9 is released, it is recommended to announce that users should upgrade to ESM, CJS may be no longer supported in future major versions.

## Drawbacks

<!--
Why should we *not* do this? Consider why adding this into ESLint
might not benefit the project or the community. Attempt to think
about any opposing viewpoints that reviewers might bring up.

Any change has potential downsides, including increased maintenance
burden, incompatibility with other tools, breaking existing user
experience, etc. Try to identify as many potential problems with
implementing this RFC as possible.
-->
1. Compatibility issues, see below.

## Backwards Compatibility Analysis
Copy link
Sponsor Member

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;

Copy link

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

Copy link
Member

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.

Copy link
Sponsor

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 :-)

Copy link

@simlu simlu Jul 20, 2022

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)


<!--
How does this change affect existing ESLint users? Will any behavior
change for them? If so, how are you going to minimize the disruption
to existing users?
-->

1. For users of the ESLint CLI: there is no impact and users can write configuration files using ESM/CJS.
2. For users of the Node.js API:
* ESLint v9: Will support ESM/CJS, so there will be no compatibility issues for users.
* ESLint 1x: CJS will no longer be supported. At this point developers can:

a. convert to esm.
```diff
- var { ESLint } = require("eslint");
+ import { ESLint } from "eslint";
```

b. move to async.
```diff
- var { ESLint } = require("eslint");
+ var { ESLint } = await import("eslint");
```

c. stay previous version util they can convert to esm.

d. compile ESLint to CJS, but it's simply not tenable or safe to transpile code one didn't author.
## Alternatives

<!--
What other designs did you consider? Why did you decide against those?

This section should also include prior art, such as whether similar
projects have already implemented a similar feature.
-->
1. esm-only in eslint v9?

While some popular Node.js packages like `chalk`, `vite`, have been ESM-only, the eslint community hasn't shifted significantly enough to warrant a change to ESM-only quite yet.

## Open Questions

<!--
This section is optional, but is suggested for a first draft.

What parts of this proposal are you unclear about? What do you
need to know before you can finalize this RFC?

List the questions that you'd like reviewers to focus on. When
you've received the answers and updated the design to reflect them,
you can remove this section.
-->
1. Do we want to bundle ESM/CJS, or just compile ESM => CJS?

To be consistent with other eslint repos, cjs will be bundled to `dist/`, while esm will not.

2. Do we want to use `*.mjs`, or adding `"type": "module"` to package.json?

I'm supportive to `*.mjs` - it allows us to gradually migration, without touching the cjs files.

3. How to keep the PRs small and focused?
One thing to be mindful of is that this ESM conversion PR is likely to be massive, touching almost every one of hundreds of file, subject to repeated merge conflicts, and taking weeks or likely even months to assemble and get the tests all working again. So, we need a careful way to keep the PRs small and focused.

As cjs can be imported in esm, we can migrate the package entry first. The main steps:
a). convert `bin/eslint.js` => `bin/eslint.mjs` (non-breaking change)
b). convert `lib/api.js` => `lib/api.mjs`, `lib/unsupported-api.js` => `lib/unsupported-api.mjs`; (breaking change)
c). others. (non-breaking change)

## Help Needed

<!--
This section is optional.

Are you able to implement this RFC on your own? If not, what kind
of help would you need from the team?
-->
I can implement it.

## Frequently Asked Questions
aladdin-add marked this conversation as resolved.
Show resolved Hide resolved

<!--
This section is optional but suggested.

Try to anticipate points of clarification that might be needed by
the people reviewing this RFC. Include those questions and answers
in this section.
-->
1. helpful toolings for the converting:
* `'import/extensions': ['error', 'always']`, (enforce file extensions) https://github.com/import-js/eslint-plugin-import/blob/main/docs/rules/extensions.md
* `'unicorn/prefer-module': 'error'` (enforces a lot of ESM syntax / disallows CJS syntax) https://github.com/sindresorhus/eslint-plugin-unicorn/blob/main/docs/rules/prefer-module.md


2. helpful (but opinionated) definitive guides for converting to esm-only:
* https://gist.github.com/sindresorhus/a39789f98801d908bbc7ff3ecc99d99c
## Related Discussions

<!--
This section is optional but suggested.

If there is an issue, pull request, or other URL that provides useful
context for this proposal, please include those links here.
-->
* https://github.com/eslint/eslint/issues/15560
* https://github.com/eslint/rfcs/pull/72