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

Deprecate CommonJS plugins #7340

Closed
Tracked by #6930
jeddy3 opened this issue Nov 15, 2023 · 23 comments · Fixed by #7353
Closed
Tracked by #6930

Deprecate CommonJS plugins #7340

jeddy3 opened this issue Nov 15, 2023 · 23 comments · Fixed by #7353
Labels
status: wip is being worked on by someone type: enhancement a new feature that isn't related to rules

Comments

@jeddy3
Copy link
Member

jeddy3 commented Nov 15, 2023

What is the problem you're trying to solve?

For 16.0.0, we've migrated our codebase to ESM (with tool-based hybrid CommonJS support) and added support for them.

We intend to be pure ESM for our next major release to update our dependencies, reduce our maintenance load and move with the times. We should deprecate support for CommonJS plugins in 16.0.0 to communicate our intent so that plugin authors are encouraged to migrate their plugins to ESM after 16.0.0 and before 17.0.0.

Ref: #7339 (comment)

What solution would you like to see?

The minimum would be changelog and migration guide entries, e.g.:


Deprecated support for CommonJS plugins

We've deprecated support for CommonJS plugins, and we'll remove support for them in our next major release.

You should update how your plugin imports and exports:

-const stylelint = require('stylelint');
+import stylelint from 'stylelint';

/* .. */

-module.exports = createPlugin(ruleName, ruleFunction);
+export default createPlugin(ruleName, ruleFunction);

You should also:

  • Add "type": "module" to your package.json.
  • Replace "main": "index.js" with "exports": "./index.js" in your package.json.
  • Update the "engines" field in package.json to Node.js 18: "node": ">=18".
  • Remove 'use strict'; from all JavaScript files.
  • Use only full relative file paths for imports: import x from '.';import x from './index.js';.
  • Use the node: protocol for Node.js built-in imports.

Ideally, we'd also generate a deprecation warning (that respects the quietDeprecationWarnings option) when commonjs plugins are used. I'm not sure if this is possible, though?

@jeddy3 jeddy3 added the status: needs discussion triage needs further discussion label Nov 15, 2023
@ybiquitous
Copy link
Member

Totally agree with this idea. 👍🏼

Ideally, we'd also generate a deprecation warning (that respects the quietDeprecationWarnings option) when commonjs plugins are used. I'm not sure if this is possible, though?

I'm also unsure about this. It should be possible if we could run any code on calling dynamic import() with the info which module type is imported, but I cannot find such a feature... 😓

@Mouvedia
Copy link
Contributor

Mouvedia commented Nov 15, 2023

and before 17.0.0

I am probably in the minority but I would strongly prefer if it was before 18.0.0.
The reason being that there are many plugins that are not maintained anymore which are mature and stable.
It will require a lot of effort/time to transfer the ownership or create new packages for those.

we'd also generate a deprecation warning

👍

@ybiquitous
Copy link
Member

there are many plugins that are not maintained anymore which are mature and stable

Yeah, this is our headache...

@newives
Copy link

newives commented Nov 15, 2023

If developers are forced to use esm, it should be announced to developers a long time in advance. For example, stylelint already supports export default{} and most plug-in developers can upgrade without changing too much code. Maybe stylelint16 is a good transition because it supports both CommonJS and esm. After all, most of them have been developed so far. It will take a long time to completely move towards esm.

@jeddy3 jeddy3 mentioned this issue Nov 15, 2023
10 tasks
@jeddy3
Copy link
Member Author

jeddy3 commented Nov 15, 2023

It should be possible if we could run any code on calling dynamic import() with the info which module type is imported, but I cannot find such a feature...

Yes, I assumed that'd be possible too. However, if it's not, then we'll be OK with a changelog and migration guide entry.

I'll label as ready to implement and add to #6930.

@jeddy3 jeddy3 added status: ready to implement is ready to be worked on by someone type: enhancement a new feature that isn't related to rules and removed status: needs discussion triage needs further discussion labels Nov 15, 2023
@ybiquitous
Copy link
Member

FYI, since dynamic import() can load a CommonJS module, CJS plugins may work after we remove support for them (unless we don't change the API).

@newives
Copy link

newives commented Nov 16, 2023

It would be great if the cjs plugin could be supported. After all, stylelint has a huge plugin ecosystem. If it is mandatory to use esm to write plugins, it will probably take months or years to migrate.

eslint, prettier and commitlint both support esm and are also compatible with cjs plug-in

@jeddy3
Copy link
Member Author

jeddy3 commented Nov 16, 2023

My understanding is that the deprecation is needed because plugins, unlike formatters and custom syntaxes, import Stylelint to use utilities like report. I believe eslint, prettier and other plugins don't do this. In hindsight, we should've taken a similar approach when we designed the plugin system many years ago.

For example, the following won't work when Stylelint is pure ESM:

// myplugin.cjs
const stylelint = require('stylelint');

const {
  createPlugin,
  utils: { report, ruleMessages, validateOptions },
} = stylelint;

*/ .. */

I think our options are:

  • deprecate cjs plugins to encourage the migration of plugins to ESM
  • rearchitect our plugin system so that plugins don't need to import stylelint

Can anyone think of other options? If not, I'm in favour of the first option because the whole JavaScript ecosystem is shifting towards ESM, and we may not have the resources to redesign our plugin system. And even if we did, it may only be possible with breaking changes.

@firefoxic
Copy link
Contributor

firefoxic commented Nov 16, 2023

rearchitect our plugin system so that plugins don't need to import stylelint

Oh! I just today replaced those utilities with an import from Stylelint 😅

It's easier for me, I can just revert the commit. But for old plugins, authors will have to move them manually.

What about stylelint.createPlugin and stylelint.lint? Or is there no problem with them being "imported" into a CJS plugin?

@Mouvedia
Copy link
Contributor

Mouvedia commented Nov 16, 2023

Replace "main": "index.js" with "exports": "./index.js" in your package.json.

This should be removed. It's not necessary; it's equivalent but slightly less compatible with other tools that might only pick up main. Separating MUST and SHOULD helps in a migration process.

If you wanna be thorough, you may add a note saying that a dual ESM/CJS plugin that would target stylelint 14 and ≥15 would require both exports and main. The reason being #6477, which pushed the minimum node version above 13.
i.e. since exports was introduced in version 13 a plugin supporting only stylelint ≥15 would only require one or the other
Most of the time it's version-dependent, so the note is hardly relevant.
i.e. the previous version supports 15, the current 16, etc. (but not several versions at once)

@Mouvedia
Copy link
Contributor

Mouvedia commented Nov 16, 2023

rearchitect our plugin system so that plugins don't need to import stylelint

  • If it's not a big refactor, it's worth exploring.
    Switching to ESM is finicky. Hoping that the whole ecosystem of plugins will be migrated during one major life-cycle is unrealistic IMHO.
  • If it's too complicated, the deprecation should last 2 majors versions else we risk to have users not upgrading due to a lack of ressource required to fork/release the unmaintained packages internally.

@ybiquitous
Copy link
Member

ybiquitous commented Nov 17, 2023

First, I'm in favor of switching Stylelint to pure ESM in 17.0.0 (next major) in order to:

  • keep dependencies up-to-date
  • reduce the package size by .cjs files
  • reduce maintainer's issue triage work due to CJS
  • etc...

As @jeddy3 commented, CJS plugins won't work with pure ESM Stylelint since CJS modules cannot import ESM modules. Sorry, I missed it.

  • deprecate cjs plugins to encourage the migration of plugins to ESM

I lean toward this deprecation, although I still understand the difficulty of rewriting the existing CJS plugins to ESM.

Perhaps providing a rewriting tool to ESM might help ease the pain of plugin authors...

  • rearchitect our plugin system so that plugins don't need to import stylelint

I doubt this policy change because:

  • this rearchitecting also requires the rewrite of the existing plugins
  • common utilities like @eslint-community/eslint-utils are still necessary to write a plugin efficiently
  • we should spend fewer resources on new rules for new syntax rather than internals

@ybiquitous
Copy link
Member

As @Mouvedia suggested on #7340 (comment), I agree with leaving the main field, too.

The Node.js document says below:

compatibility can be achieved by including the "main" field alongside "exports" pointing to the same module:

{
 "main": "./index.js",
 "exports": "./index.js"
}

Instead of replacing main, how about suggesting just adding exports?

@ybiquitous
Copy link
Member

For your information, Vite 5 (which is released today) deprecates CJS Node API.

Here's how to show the deprecation warning:
https://github.com/vitejs/vite/blob/2844a0f66e13006d7624ffb564a63af0f20276fa/packages/vite/index.cjs#L3

@jeddy3
Copy link
Member Author

jeddy3 commented Nov 17, 2023

@ybiquitous It looks like we agree on switching to ESM in 17, and deprecating CJS plugins rather than attempting to rearchitect the plugin system. All your points make sense to me.

Perhaps providing a rewriting tool to ESM might help ease the pain of plugin authors...

We should do our best to help plugin authors migrate, but a tool (even though ideal) may not be achievable. We can investigate the possibility, though, if anyone has time.

Getting the migration guide in a good place would be great. It doesn't need to be perfect because we can improve it as plugin authors provide feedback on what obstacles they encounter. They'll also have plenty of time to migrate, as it'll be at least a few months (or even a year) before we release the next major version.

When I was migrating stylelint-scales, switching code to ESM import and export statements wasn't an issue, whereas getting Jest to work was a pain point. Let's try to improve that experience, as most plugin authors use our Jest preset.

We can add using runner: "jest-light-runner" as part of the migration guide. We could also investigate the possibility of providing an alternative test rule runner. For example, a stylelint-test-rule-node package that exports a testRule() function using the built-in node:test and node:assert. That should work with ESM code out of the box.

It should ideally be close to a drop-in replacement for most plugins:

+ import { testRule } from "stylelint-test-rule-node"

testRule({ /* .. */ });
{
  "devDependencies": {
-    "jest": "^29.7.0",
-    "jest-preset-stylelint": "^6.3.2",
  },
 "scripts": {
-     "test": "jest",
+    "test": "node --test",
  }
}

Has anyone else encountered pain points while migrating their plugin to ESM to test the prerelease? If so, please share so that we can ensure the first version of the migration guide covers these.

@firefoxic
Copy link
Contributor

providing an alternative test rule runner?

I tried a couple test suites to rewrite to node:test in my plugin. It was surprisingly easy. And everything worked well (and also several times faster). Although I took some of the simplest tests (without a lot of dependencies and complex code), but I think there will be no problems there too. I think the main problem will be the routine of replacing code fragments in the same way.

@firefoxic
Copy link
Contributor

firefoxic commented Nov 17, 2023

Has anyone else encountered pain points while migrating their plugin to ESM to test the prerelease? If so, please share so that we can ensure the first version of the migration guide covers these.

I had a pretty specific problem, hardly relevant to anyone who needs to really put a lot of effort into rewriting cjs code in esm. The thing is that I already had the plugin in esm, and only for stylelint<16 I had to compile to cjs with babel. And when I tried to make at least an alpha-version for those who need cjs — it didn't work, because stylelint expected default immediately in the root of the plugin object, but I had the necessary default in a nested default inside an array (I don't remember exactly) and I got an error. In my case the fix would be more expensive than the profit, so I just deleted babel and made a new pre-release with already normally working esm.

Most plugins have cjs in the source code itself, not built by babel, rollup or whatever. So this problem should not occur.

@firefoxic
Copy link
Contributor

+ import testRule from "stylelint-test-rule-node"

testRule({ /* .. */ });

That would be awesome 😍

@ybiquitous
Copy link
Member

The idea of a new test helper using node:test sounds great! 👍🏼

@Mouvedia
Copy link
Contributor

Mouvedia commented Nov 17, 2023

Instead of replacing main, how about suggesting just adding exports?

That's OK. What I am advocating is to separate points that are MUST from the ones that are MAY in the migration guide.
e.g. exports addition being one of the MAY

They'll also have plenty of time to migrate, as it'll be at least a few months (or even a year) before we release the next major version.

If we can stretch to more than a year, it might be enough time if we have proper warnings which respect --quiet-deprecation-warnings.

…whereas getting Jest to work was a pain point. Let's try to improve that experience…
For example, a stylelint-test-rule-node package that exports a testRule() function using the built-in node:test and node:assert.

👍

@ybiquitous
Copy link
Member

ybiquitous commented Nov 17, 2023

If node:test would be sufficient, we might be able to provide new test helpers similar to jest-preset-stylelint by the stylelint package itself. For example:

// index.test.js
import { testRule, testRuleConfigs } from "stylelint/test";

import plugin from "./index.js";

const { rule: { messages, ruleName } } = plugin;

testRule({
  plugins: [rule],
  ruleName,
  // ...
});

@JounQin
Copy link
Member

JounQin commented Nov 17, 2023

Is that a good idea to install test-only related codes for all regular users?

@jeddy3
Copy link
Member Author

jeddy3 commented Nov 17, 2023

In the example above, the plugin object (rather than a locator to it) is passed to the plugins: [] configuration object. Let's include this pattern in the migration and writing plugins guides as part of the PR for this issue. And also update the jest-preset-stylelint README at the same time.

Ref: jeddy3/stylelint-scales#94 (comment)

I think the deprecation is ready to be worked on if anyone has time. As Jest now works with some minor changes (that we can communicate in the migration guide), creating a testRule using node:test shouldn't block us from releasing 16.0.0. It'd be really nice to have, though!

Jest migration changes for the guide:

// jest.setup.js
- const { getTestRule } = require("jest-preset-stylelint");
+ import { getTestRule } from "jest-preset-stylelint";

- global.testRule = getTestRule({ plugins: [plugin] });
+ global.testRule = getTestRule();
// package.json
{ 
  "scripts": {
-    "test": "jest",
+    "test": "cross-env NODE_OPTIONS=--experimental-vm-modules jest --runInBand"
  }
}
// plugin.test.js
import plugin from "../index.js";
const {
  rule: { messages, ruleName },
} = plugin;

testRule({
+. plugins: [rule]
  /* .. */
});

Is that a good idea to install test-only related codes for all regular users?

That's a good point. Maybe a separate package is better.

@ybiquitous ybiquitous removed the status: ready to implement is ready to be worked on by someone label Nov 21, 2023
@ybiquitous ybiquitous added the status: wip is being worked on by someone label Nov 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: wip is being worked on by someone type: enhancement a new feature that isn't related to rules
Development

Successfully merging a pull request may close this issue.

6 participants