Build CJS files via Rollup #7037

ybiquitous · 2023-07-03T13:53:57Z

Which issue, if any, is this issue related to?

Ref #5291

Is there anything in the PR that needs further explanation?

Steps for this PR:

Run npm i -D rollup
Create rollup.config.mjs
Rewrite .js to .mjs in ESM
- Note that this PR includes just a few .mjs. Following-up PRs will gradually migrate .js files to ESM.

Discussion points:

Should we commit .cjs files? => Yes.

See https://rollupjs.org/

Note: This PR is still a draft, but I welcome any feedback. 👍🏼

I'll quickly explain the methodology for this PR: 1. remove each `lib/rules/*` for each deprecated test 2. remove each (now invalid) path from `lib/rules/index.js` 3. fix all broken tests, which fall into one of several categories: - tests that use a deprecated rule as an arbitrary rule; to resolve this, I just picked a different rule (usually `color-named` for `color-hex-case` and `number-max-precision` for `indentation`) - one of these is about a custom plugin, so I've rewritten the plugin - tests that are *about* deprecated rules; I added a `.skip` (have left comments in the review) - fs tests that use deprecated rules; I removed these from the config, then updated the snapshot. - the zen garden one doesn't work "out of the box" anymore, i.e. the snapshot has nontrivially changed because of indentation/spacing rules - `indentation`-specific behaviour (it being last); I deleted these! 4. remove all documentation related to removed rules (mostly `indentation`) 5. remove all unused utilities (via code coverage, not manual inspection)

Ref: #6979 (comment) Re-running the script indicates no newly-unused files: ```sh $ node scripts/find-unused-modules.mjs ```

Node.js 16.13.0 is the first version of Node.js 16.x LTS series. See https://nodejs.org/en/blog/release/v16.13.0

changeset-bot · 2023-07-03T13:54:01Z

⚠️ No Changeset found

Latest commit: e78b70f

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

package.json

romainmenke · 2023-07-03T17:13:53Z

Should we commit .cjs files?

For everything under @csstools/postcss-plugins we use this approach :

every build output is committed
CI rebuilds changed plugins and fails if the build output in git doesn't match a fresh build

this steps are fairly complicated because it is a monorepo housing many different packages

We switched to this strategy because building every plugin took to long and we didn't want CI duration to be a factor when deciding to create something new.

In practice we found that it is very rare that more than one person is changing the same plugin at the same time. So we rarely have conflicts in build outputs. I can imagine that the same is true for Stylelint rules.

We also added some git attributes to make it easier to resolve conflicts : https://github.com/csstools/postcss-plugins/blob/main/.gitattributes#L3-L6

When there is a conflict it will always result in a broken build output file, but that is fine.
This allows us to stay focused on the merge conflicts in the source files.
After resolving those we rebuild and commit the new outputs.

) `mergeTestDescriptions()` is used by deprecated rules. So I've created this commit from the `v16` branch, instead of the `main` branch. This change enables the `esModuleInterop` flag to prevent the following TypeScript error: ``` lib/testUtils/mergeTestDescriptions.mjs:1:8 - error TS1259: Module '"/Users/masafumi.koba/git/stylelint/stylelint/node_modules/deepmerge/index"' can only be default-imported using the 'esModuleInterop' flag 1 import merge from 'deepmerge'; ~~~~~ node_modules/deepmerge/index.d.ts:20:1 20 export = deepmerge; ~~~~~~~~~~~~~~~~~~~ This module is declared with 'export =', and can only be used with a default import when using the 'esModuleInterop' flag. ```

ybiquitous · 2023-07-04T00:58:35Z

@romainmenke Thanks for providing the actual example. There are many pros and cons here.

Checking built CJS files' validity on CI sounds good. 👍🏼

I'm concerned about verbose Git commit history. 🤔

See https://rollupjs.org/

ybiquitous · 2023-07-04T13:07:10Z

I found an advantage in committing .cjs files. Committed .cjs enables people to install stylelint from GitHub, for example:

npm install github:stylelint/stylelint

The second advantage is that we can easily compare .mjs and .cjs without building, for example, on GitHub. I'm leaning towards committing .cjs now, though we can go back before releasing v16.

scripts/build-check.mjs

ybiquitous · 2023-07-04T14:18:50Z

Now ready for review!

package.json

rollup.config.mjs

I'll quickly explain the methodology for this PR: 1. remove each `lib/rules/*` for each deprecated test 2. remove each (now invalid) path from `lib/rules/index.js` 3. fix all broken tests, which fall into one of several categories: - tests that use a deprecated rule as an arbitrary rule; to resolve this, I just picked a different rule (usually `color-named` for `color-hex-case` and `number-max-precision` for `indentation`) - one of these is about a custom plugin, so I've rewritten the plugin - tests that are *about* deprecated rules; I added a `.skip` (have left comments in the review) - fs tests that use deprecated rules; I removed these from the config, then updated the snapshot. - the zen garden one doesn't work "out of the box" anymore, i.e. the snapshot has nontrivially changed because of indentation/spacing rules - `indentation`-specific behaviour (it being last); I deleted these! 4. remove all documentation related to removed rules (mostly `indentation`) 5. remove all unused utilities (via code coverage, not manual inspection)

Ref: #6979 (comment) Re-running the script indicates no newly-unused files: ```sh $ node scripts/find-unused-modules.mjs ```

To prevent `npm run build` for consumers.

I'll quickly explain the methodology for this PR: 1. remove each `lib/rules/*` for each deprecated test 2. remove each (now invalid) path from `lib/rules/index.js` 3. fix all broken tests, which fall into one of several categories: - tests that use a deprecated rule as an arbitrary rule; to resolve this, I just picked a different rule (usually `color-named` for `color-hex-case` and `number-max-precision` for `indentation`) - one of these is about a custom plugin, so I've rewritten the plugin - tests that are *about* deprecated rules; I added a `.skip` (have left comments in the review) - fs tests that use deprecated rules; I removed these from the config, then updated the snapshot. - the zen garden one doesn't work "out of the box" anymore, i.e. the snapshot has nontrivially changed because of indentation/spacing rules - `indentation`-specific behaviour (it being last); I deleted these! 4. remove all documentation related to removed rules (mostly `indentation`) 5. remove all unused utilities (via code coverage, not manual inspection)

Ref: #6979 (comment) Re-running the script indicates no newly-unused files: ```sh $ node scripts/find-unused-modules.mjs ```

Node.js 16.13.0 is the first version of Node.js 16.x LTS series. See https://nodejs.org/en/blog/release/v16.13.0

) `mergeTestDescriptions()` is used by deprecated rules. So I've created this commit from the `v16` branch, instead of the `main` branch. This change enables the `esModuleInterop` flag to prevent the following TypeScript error: ``` lib/testUtils/mergeTestDescriptions.mjs:1:8 - error TS1259: Module '"/Users/masafumi.koba/git/stylelint/stylelint/node_modules/deepmerge/index"' can only be default-imported using the 'esModuleInterop' flag 1 import merge from 'deepmerge'; ~~~~~ node_modules/deepmerge/index.d.ts:20:1 20 export = deepmerge; ~~~~~~~~~~~~~~~~~~~ This module is declared with 'export =', and can only be used with a default import when using the 'esModuleInterop' flag. ```

jeddy3 · 2023-07-09T16:17:18Z

@ybiquitous Thank you for already moving the migration to ESM along so far!

I've been catching up on the discussions in #5291, and I wanted to check how we feel about #5291 (comment):

Are you sure it's ok for some plugins to import from the ESM Stylelint modules, and some to require from the CJS ones, at the same time when linting a project occurs? That won't cause problems?

Is there any way for us to test this?

ybiquitous · 2023-07-10T13:47:57Z

@jeddy3 Good point. I think we should add a system test case to test importing ESM and CJS plugins in one config. For example:

{
	"plugins": ["./plugin.cjs", "./plugin.mjs"],
	"rules": {
		"cjs/foo": true,
		"esm/foo": true
	}
}

However, please note that this case should fail now since the current Stylelint supports only require(), not import():

stylelint/lib/augmentConfig.js

Line 319 in 6765374

pluginImport = require(pluginLookup);

I think we need to change require() to import() (dynamic import) to support both ESM and CJS. This means we need to introduce a Promise-based API, keeping backward compatibility as possible.

Any thoughts?

jeddy3 · 2023-07-10T17:54:20Z

I think we need to change require() to import() (dynamic import) to support both ESM and CJS. This means we need to introduce a Promise-based API, keeping backward compatibility as possible.

I believe so. We can see if keeping backward compatibility is possible with our resources.

How do we move forward with this pull request, e.g. shall we merge so we can keep moving forward with trying dynamic imports to uncover any potential issues?

(I'm going offline for a week for a festival. I've approved the PR in case this is the route we want to go down, and I don't want to block you.)

jeddy3

Many thanks again for driving the migration to ESM forward!

ybiquitous · 2023-07-10T23:35:38Z

How do we move forward with this pull request, e.g. shall we merge so we can keep moving forward with trying dynamic imports to uncover any potential issues?

I suggest gradually rewriting the current code to ESM to make each PR easier to review. I guess the most difficult part may be around importing (e.g. require() or import-lazy)

See https://rollupjs.org/

mattxwang and others added 4 commits July 3, 2023 20:39

Remove unused modules (from deprecated rules) (#7003)

c34c1c6

Ref: #6979 (comment) Re-running the script indicates no newly-unused files: ```sh $ node scripts/find-unused-modules.mjs ```

Remove Node.js 14 support (#7020)

07eb5eb

Node.js 16.13.0 is the first version of Node.js 16.x LTS series. See https://nodejs.org/en/blog/release/v16.13.0

Remove isStandardSyntaxMediaFeatureName utility (#7023)

b55c5d3

ybiquitous force-pushed the esm-rollup branch 2 times, most recently from 0c9dae3 to 188b9f6 Compare July 3, 2023 14:01

ybiquitous commented Jul 3, 2023

View reviewed changes

package.json Outdated Show resolved Hide resolved

ybiquitous force-pushed the esm-rollup branch from 188b9f6 to e2d0016 Compare July 3, 2023 14:09

Mouvedia reviewed Jul 3, 2023

View reviewed changes

package.json Outdated Show resolved Hide resolved

Build CJS files via Rollup

9a8c52c

See https://rollupjs.org/

ybiquitous force-pushed the esm-rollup branch from e2d0016 to 9a8c52c Compare July 4, 2023 00:58

ybiquitous added 4 commits July 4, 2023 23:01

Check built files on CI

4dcd6a1

Migrate lib/constants to ESM

ceaf3f3

Add missing lib/constants.cjs

3ea2cbf

Run npm run build-check on CI

681878a

ybiquitous commented Jul 4, 2023

View reviewed changes

scripts/build-check.mjs Outdated Show resolved Hide resolved

ybiquitous marked this pull request as ready for review July 4, 2023 14:16

Mouvedia reviewed Jul 4, 2023

View reviewed changes

package.json Show resolved Hide resolved

Mouvedia reviewed Jul 4, 2023

View reviewed changes

rollup.config.mjs Show resolved Hide resolved

Mouvedia reviewed Jul 4, 2023

View reviewed changes

rollup.config.mjs Show resolved Hide resolved

mattxwang force-pushed the v16 branch 2 times, most recently from 0542997 to 7176474 Compare July 4, 2023 21:16

mattxwang added 2 commits July 7, 2023 07:41

Remove unused modules (from deprecated rules) (#7003)

2806bd3

Ref: #6979 (comment) Re-running the script indicates no newly-unused files: ```sh $ node scripts/find-unused-modules.mjs ```

ybiquitous and others added 7 commits July 8, 2023 23:04

Merge branch 'v16' into esm-rollup

975b0bc

Run npm run build on prepare instead of postinstall

66fa3a6

To prevent `npm run build` for consumers.

Remove unused modules (from deprecated rules) (#7003)

6ea6b18

Ref: #6979 (comment) Re-running the script indicates no newly-unused files: ```sh $ node scripts/find-unused-modules.mjs ```

Remove Node.js 14 support (#7020)

0fb152e

Node.js 16.13.0 is the first version of Node.js 16.x LTS series. See https://nodejs.org/en/blog/release/v16.13.0

Remove isStandardSyntaxMediaFeatureName utility (#7023)

2eb9a1b

ybiquitous force-pushed the v16 branch from 2a68c97 to fb94d5e Compare July 8, 2023 15:04

Merge branch 'v16' into esm-rollup

84afc93

jeddy3 approved these changes Jul 10, 2023

View reviewed changes

ybiquitous force-pushed the v16 branch from fb94d5e to bd176da Compare July 13, 2023 12:39

Merge branch 'v16' into esm-rollup

e78b70f

ybiquitous merged commit e0b80ec into v16 Jul 13, 2023
15 checks passed

ybiquitous deleted the esm-rollup branch July 13, 2023 13:01

ybiquitous added a commit that referenced this pull request Jul 20, 2023

Build CJS files via Rollup (#7037)

c0c6ee8

See https://rollupjs.org/

ybiquitous added a commit that referenced this pull request Jul 24, 2023

Build CJS files via Rollup (#7037)

9d8188d

See https://rollupjs.org/

ybiquitous added a commit that referenced this pull request Jul 25, 2023

Build CJS files via Rollup (#7037)

1ae8984

See https://rollupjs.org/

ybiquitous added a commit that referenced this pull request Jul 26, 2023

Build CJS files via Rollup (#7037)

5d5c159

See https://rollupjs.org/

ybiquitous added a commit that referenced this pull request Jul 26, 2023

Build CJS files via Rollup (#7037)

3817658

See https://rollupjs.org/

ybiquitous added a commit that referenced this pull request Jul 27, 2023

Build CJS files via Rollup (#7037)

dde685b

See https://rollupjs.org/

ybiquitous added a commit that referenced this pull request Aug 12, 2023

Build CJS files via Rollup (#7037)

2a34e34

See https://rollupjs.org/

ybiquitous added a commit that referenced this pull request Aug 14, 2023

Build CJS files via Rollup (#7037)

c8c8f72

See https://rollupjs.org/

ybiquitous added a commit that referenced this pull request Aug 27, 2023

Build CJS files via Rollup (#7037)

cfe3cd5

See https://rollupjs.org/

ybiquitous added a commit that referenced this pull request Aug 27, 2023

Build CJS files via Rollup (#7037)

7e28f14

See https://rollupjs.org/

ybiquitous added a commit that referenced this pull request Sep 28, 2023

Build CJS files via Rollup (#7037)

60ae32b

See https://rollupjs.org/

ybiquitous added a commit that referenced this pull request Oct 19, 2023

Build CJS files via Rollup (#7037)

3b9f78b

See https://rollupjs.org/

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Build CJS files via Rollup #7037

Build CJS files via Rollup #7037

ybiquitous commented Jul 3, 2023 •

edited

changeset-bot bot commented Jul 3, 2023 •

edited

romainmenke commented Jul 3, 2023

ybiquitous commented Jul 4, 2023

ybiquitous commented Jul 4, 2023

ybiquitous commented Jul 4, 2023

jeddy3 commented Jul 9, 2023

ybiquitous commented Jul 10, 2023

jeddy3 commented Jul 10, 2023

jeddy3 left a comment

ybiquitous commented Jul 10, 2023

Build CJS files via Rollup #7037

Build CJS files via Rollup #7037

Conversation

ybiquitous commented Jul 3, 2023 • edited

changeset-bot bot commented Jul 3, 2023 • edited

⚠️ No Changeset found

romainmenke commented Jul 3, 2023

ybiquitous commented Jul 4, 2023

ybiquitous commented Jul 4, 2023

ybiquitous commented Jul 4, 2023

jeddy3 commented Jul 9, 2023

ybiquitous commented Jul 10, 2023

jeddy3 commented Jul 10, 2023

jeddy3 left a comment

Choose a reason for hiding this comment

ybiquitous commented Jul 10, 2023

ybiquitous commented Jul 3, 2023 •

edited

changeset-bot bot commented Jul 3, 2023 •

edited