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
Comments
Totally agree with this idea. 👍🏼
I'm also unsure about this. It should be possible if we could run any code on calling dynamic |
I am probably in the minority but I would strongly prefer if it was before 18.0.0.
👍 |
Yeah, this is our headache... |
If developers are forced to use |
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. |
FYI, since dynamic |
It would be great if the
|
My understanding is that the deprecation is needed because plugins, unlike formatters and custom syntaxes, import Stylelint to use utilities like 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:
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. |
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 |
This should be removed. It's not necessary; it's equivalent but slightly less compatible with other tools that might only pick up 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 |
|
First, I'm in favor of switching Stylelint to pure ESM in 17.0.0 (next major) in order to:
As @jeddy3 commented, CJS plugins won't work with pure ESM Stylelint since CJS modules cannot import ESM modules. Sorry, I missed it.
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...
I doubt this policy change because:
|
As @Mouvedia suggested on #7340 (comment), I agree with leaving the The Node.js document says below:
Instead of replacing |
For your information, Vite 5 (which is released today) deprecates CJS Node API. Here's how to show the deprecation warning: |
@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.
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 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. |
I tried a couple test suites to rewrite to |
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 Most plugins have cjs in the source code itself, not built by babel, rollup or whatever. So this problem should not occur. |
That would be awesome 😍 |
The idea of a new test helper using |
That's OK. What I am advocating is to separate points that are MUST from the ones that are MAY in the migration guide.
If we can stretch to more than a year, it might be enough time if we have proper warnings which respect
👍 |
If // index.test.js
import { testRule, testRuleConfigs } from "stylelint/test";
import plugin from "./index.js";
const { rule: { messages, ruleName } } = plugin;
testRule({
plugins: [rule],
ruleName,
// ...
}); |
Is that a good idea to install test-only related codes for all regular users? |
In the example above, the plugin object (rather than a locator to it) is passed to the 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 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]
/* .. */
});
That's a good point. Maybe a separate package is better. |
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:
You should also:
"type": "module"
to your package.json."main": "index.js"
with"exports": "./index.js"
in your package.json."engines"
field in package.json to Node.js 18:"node": ">=18"
.'use strict';
from all JavaScript files.import x from '.';
→import x from './index.js';
.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?The text was updated successfully, but these errors were encountered: