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

Move to ESM #5291

Closed
Tracked by #5205 ...
jeddy3 opened this issue May 12, 2021 · 59 comments · Fixed by #7303
Closed
Tracked by #5205 ...

Move to ESM #5291

jeddy3 opened this issue May 12, 2021 · 59 comments · Fixed by #7303
Labels
help wanted is likely non-trival and help is wanted status: wip is being worked on by someone type: enhancement a new feature that isn't related to rules type: umbrella a group of related issues

Comments

@jeddy3
Copy link
Member

jeddy3 commented May 12, 2021

We'll need to:

  1. following the ESM migration guide:
  2. replace lazy import with dynamic imports

How can I move my CommonJS project to ESM?

  • Add "type": "module" to your package.json.
  • Replace "main": "index.js" with "exports": "./index.js" in your package.json.
  • Remove 'use strict'; from all JavaScript files.
  • Replace all require()/module.export with import/export.
  • Use only full relative file paths for imports: import x from '.';import x from './index.js';.
  • If you have a TypeScript type definition (for example, index.d.ts), update it to use ESM imports/exports.
  • Optional but recommended, use the node: protocol for imports.
@jeddy3 jeddy3 added the status: ready to implement is ready to be worked on by someone label May 12, 2021
@jeddy3 jeddy3 added this to the future-major milestone May 12, 2021
@jeddy3 jeddy3 mentioned this issue May 12, 2021
30 tasks
@ybiquitous
Copy link
Member

@jeddy3 Should we create a long-lived branch such as esm?

@jeddy3
Copy link
Member Author

jeddy3 commented May 12, 2021

Yep. I've created a v14 branch for the issues listed in #5205 (comment)

Replace all require()/module.export with import/export.

We can use https://github.com/knowbee/rona for this.

Optional but recommended, use the node: protocol for imports.

Find and replace should suffice for this.

replace lazy import with dynamic imports

This is probably the trickiest aspect of this issue.

@hudochenkov
Copy link
Member

Shall we wait with ESM until Jest has non-experimental support for it?

@jeddy3
Copy link
Member Author

jeddy3 commented May 12, 2021

Let's transpile for Jest for now (Method B or C):
https://bl.ocks.org/rstacruz/511f43265de4939f6ca729a3df7b001c

So that we can update our growing list of dependencies that require ESM.

We can move from transpiling to native ESM Jest when it's available.

@jeddy3
Copy link
Member Author

jeddy3 commented May 15, 2021

This issue is now unblocked and can be started on.

We can use putout to do the bulk of the import conversion.

Putting a .putout.json config in the root:

{
    "rules": {
        "convert-commonjs-to-esm/require": "on",
        "convert-commonjs-to-esm/exports": "on",
        "convert-commonjs-to-esm/commons": "off",
        "apply-destructuring/array": "off",
        "convert-to-arrow-function": "off",
        "apply-destructuring/object": "off",
        "remove-iife": "off",
        "simplify-ternary": "off",
        "merge-if-statements": "off",
        "apply-optional-chaining": "off",
        "remove-boolean-from-assertions": "off",
        "convert-for-each-to-for-of": "off",
        "regexp/optimize": "off",
        "regexp/remove-useless-regexp": "off",
        "convert-equal-to-strict-equal": "off",
        "regexp/remove-useless-group": "off",
        "convert-array-copy-to-slice": "off",
        "remove-useless-variables/rename": "off",
        "remove-useless-operand": "off",
        "remove-useless-array-constructor": "off",
        "remove-useless-type-convertion/named": "off",
        "convert-assignment-to-comparison": "off",
        "convert-for-to-for-of/length": "off",
        "apply-numeric-separators": "off",
        "remove-useless-variables/remove": "off",
        "remove-console": "off",
        "remove-useless-spread/object": "off",
        "convert-template-to-string": "off",
        "remove-unused-variables": "off",
        "nodejs/convert-promisify-to-fs-promises": "off",
        "remove-useless-await": "off"
    },
    "processors": [
        ["markdown", "off"],
        ["css", "off"],
        ["yaml", "off"],
        ["ignore", "off"]
    ]
}

And running:

npx putout lib/**/*.js

I suggest we tackle this in stages. First, we move to import statements. Then let's avoid where we use dynamic requires for points of extension (plugins, shared configs, custom syntax etc) and move rules and formatters to static imports. Once we have this core functionality working, let's investigate dynamic imports for points of extension. Finally, we can look into whether dynamic imports can be used for the built-in rules and formatters to improve performance.

I may have time to get the ball rolling with the move to import statements this afternoon. That'll give us a foundation to get a sense of how much work is involved in this move to ESM.

@hudochenkov
Copy link
Member

Maybe we should wait with this until PostCSS 8 migration is done #4942? The reason is PostCSS 8 has ESM exports, but PostCSS 7 — not.

@jeddy3
Copy link
Member Author

jeddy3 commented May 15, 2021

Sounds good to me. I was dipping into it just now, but I think this and #4942 are going to require more changes to the internals than I originally thought. It's probably best we focus on #4942 first, and then migrate to ESM (to update all ESM-only dependencies) to avoid conflicts.

@hudochenkov
Copy link
Member

Another important thing that maybe important to postpone ESM migration for a while. With PostCSS 8 there is a big chance, that stylelint plugins should be a little bit different and use PostCSS 8 visitors API. That could be challenging for plugin developers.

If stylelint would be ESM, plugins also have to be ESM, because every plugin imports stylelint. And CommonJS can't import ESM.

Combining this two big changes at the same time could be very problematic for plugin author.

Maybe for v14 let's stay on CommonJS. And in v15 we move to ESM. We could do v15 right after v14. It will allow plugin authors first to migrate to PostCSS 8 and new plugin format. Test it, and then migrate to ESM.

@ybiquitous
Copy link
Member

FYI. There is a way of "Conditional exports" for that.

  • Provide ESM mainly
  • Provide also CJS for backward compatibility

We can generate CJS files via a transpiler like rollup.js.

@jeddy3
Copy link
Member Author

jeddy3 commented May 19, 2021

@hudochenkov Those are all good points.

It will allow plugin authors first to migrate to PostCSS 8 and new plugin format. Test it, and then migrate to ESM.

I originally thought that, in the long run, it'll be better for users if we draw a clean line in the sand:

  • new world is ESM and PostCSS 8
  • old world is commonjs and PostCSS 7

That'd help users navigate compatible and incompatible plugins and tasks runners. However, I agree that this may be too much to ask of plugin and task runner authors in the short term.

@ybiquitous' suggestion of conditional exports may be the appropriate middle ground. Going pure ESM is probably more suitable for libraries (like the ones we import) than tools like Stylelint at this point. We can take a dual approach for the next year and then see how the dust settles.

This would allow us to move to ESM ourselves in 14.0.0 and update the 8 of our dependencies that are pure ESM already, including one that patches a vulnerability in its latest release (#5205 (comment)).

We can generate CJS files via a transpiler like rollup.js. (or esbuild)

The good news is that this is a lot more viable now with the removal of syntax (#5289), autoprefixer (#5293) and the calc parser (#4731), and by the reduction in the size of the table package (gajus/table#147 (comment)). I think we can use static imports for rules and formatters now, reserving dynamic imports/requires for points of extension. This would simplify our architecture and reduce the complexity of bundling.

If we decide to dual publish and add the infrastructure to bundle commonjs, then it's sensible to repurpose that infrastructure to bundle ESM. While investigating #2454, I found bundling to give comparable or even better performance than using lazy-import.

It's likely other issues and opportunities will emerge when we begin to more fully explore the move to ESM once all the other 14.0.0 items are complete. At that point, I think we'll be in the best position to decide on the best approach for us.

It's exciting to think what 14.0.0 is shaping up to be; the most modern, secure, sustainable and performant stylelint yet!

@hudochenkov
Copy link
Member

It's likely other issues and opportunities will emerge when we begin to more fully explore the move to ESM once all the other 14.0.0 items are complete. At that point, I think we'll be in the best position to decide on the best approach for us.

Yeah, let's see what we'll have at that time. In a good new I have my own plugin, so could get first-hand experience with PostCSS 8 and ESM before final release :) I wish I had smaller plugin :)

the most modern, secure, sustainable and performant stylelint yet!

Sounds like new iPhone announcement :)

@jeddy3
Copy link
Member Author

jeddy3 commented May 19, 2021

In a good new I have my own plugin, so could get first-hand experience with PostCSS 8 and ESM before final release :

Ditto. stylelint-scales isn't as complex as stylelint-order, but there are a few rules in it I'll need to migrate.

Sounds like new iPhone announcement :)

Ha 😃 . Building the excitement :)

@jeddy3
Copy link
Member Author

jeddy3 commented May 20, 2021

The good news is that this is a lot more viable now with the removal of syntax (#5289), autoprefixer (#5293) and the calc parser (#4731), and by the reduction in the size of the table package (gajus/table#147 (comment)).

I was curious if this was true. The results of a quick test are promising. It looks like ESBuild can bundle the v14 branch without issue. I suspect it won't have an issue with ESM either, but the proof is in the pudding.

npx esbuild ./lib/index.js --bundle --outfile=out/index.js --format=cjs --platform=node --minify --metafile=lib.json

Will result in a 900kb bundle with 100ms require times. Custom syntaxes and plugins worked fine.

If you like, you can upload the generated metafile to https://www.bundle-buddy.com to see a breakdown. The v14 branch looks good, with only lodash needing to be optimised.

@alexander-akait
Copy link
Member

We have big problem with https://github.com/stylelint/stylelint/blob/master/bin/stylelint.js#L6 (v8-compile-cache), import().then() will not work due zertosh/v8-compile-cache#30, we have the same problem in webpack

@jeddy3
Copy link
Member Author

jeddy3 commented May 26, 2021

Can we do without the package (at least until it's compatible), especially as we'll probably be bundling in 14.0.0?

@alexander-akait
Copy link
Member

Can we do without the package (at least until it's compatible), especially as we'll probably be bundling in 14.0.0?

It reduces startup time (after first running) around 20-40%, especial for custom plugins/rules/etc

We are working on https://github.com/sokra/node-voo (maybe make sense to add es modules cache support too), in near future I want to migrate webpack (i.e. webpack-cli) on this, so we will get big feedback.

Honestly, maybe we should continue to be in CJS, and use https://github.com/ai/dual-publish, the ecosystem is not quite ready yet, a lot low-level utils/packages is still on CJS, therefore, in fact, it does not bring any huge benefits. Don't forget many developers (especial boilerplates) have const stylelint = require('stylelint'); and migration may take time for them (top level await supported only Node.js v14).

top-level await is available "unflagged" in Node.js since v14.8

I personally make no small effort in ESM migrations and really look forward to it.

So Node.js 12 is transitional stage, in this stage low level tools/pages should migrate on ESM, top level projects rewrite code with thoughts future ESM using (i.e. import()/import.meta.url/etc).

Don't forget about cosmiconfig/cosmiconfig#224, JSON files should be read using fs.readFile...

Just my onion. Again, I'm not against ESM, I love ESM ⭐

ybiquitous added a commit to stylelint/eslint-config-stylelint that referenced this issue May 28, 2021
We are moving to ESM on the `stylelint/stylelint` repo:
stylelint/stylelint#5291
@jeddy3
Copy link
Member Author

jeddy3 commented May 28, 2021

Honestly, maybe we should continue to be in CJS, and use https://github.com/ai/dual-publish, the ecosystem is not quite ready yet

I think our hand is forced as 8 of our dependencies are ESM-only already. We'll need to weigh up performance costs against keeping our dependencies up to date.

Don't forget many developers (especial boilerplates) have const stylelint = require('stylelint');

The tentative plan is to author in ESM and dual publish a CJS bundle so that users can still use stylelint = require('stylelint');.

There's still a lot of other 14.0.0 tasks to complete before we attempt to migrate to ESM. Once we start the migration, I think it'll be clearer what the compromises are and what's the best way forward is.

@alexander-akait
Copy link
Member

The tentative plan is to author in ESM and dual publish a CJS bundle so that users can still use stylelint = require('stylelint');.

Sounds good, with bundler it will be easy

@ybiquitous
Copy link
Member

ybiquitous commented Sep 11, 2023

We have the following tasks left:

  • Migrate lib/rules/*.js to ESM
  • fixtures
  • Add system test cases both for ESM and CJS.

@ybiquitous
Copy link
Member

ybiquitous commented Oct 30, 2023

Progress report

I believe the migration to ESM is about to be done. Remaining work:

I'd appreciate it if you could take a look at them.

@Mouvedia
Copy link
Contributor

Add system test cases for the CJS build

Is this really a blocker?
Another one that we can also postpone, which fixes #7201, is #6836.

@ybiquitous
Copy link
Member

Is this really a blocker?

Not a blocker, but it should help for more stable releases.

Another one that we can also postpone, which fixes #7201, is #6836.

We cannot use pure ESM packages as long as we support both CJS and ESM (v16). Probably, v17 or later might be able to update them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted is likely non-trival and help is wanted status: wip is being worked on by someone type: enhancement a new feature that isn't related to rules type: umbrella a group of related issues
Development

Successfully merging a pull request may close this issue.