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 Node.js API #7353

Merged
merged 14 commits into from
Nov 27, 2023
Merged

Deprecate CommonJS Node.js API #7353

merged 14 commits into from
Nov 27, 2023

Conversation

ybiquitous
Copy link
Member

@ybiquitous ybiquitous commented Nov 21, 2023

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

Closes #7340

Is there anything in the PR that needs further explanation?

This change deprecates the use of CommonJS Node.js API and updates the migration guide to 16.0.0.

Stylelint will show the deprecation warning when using CommonJS API. This deprecation code is generated by a Rollup custom plugin.

Outdated description of STYLELINT_CJS_IGNORE_WARNING

Examples:

$ node -e 'import("stylelint")'
(no output)

$ node -e 'require("stylelint")'
The CJS build of Stylelint's Node.js API is deprecated. See https://stylelint.io/migration-guide/to-16

$ STYLELINT_CJS_IGNORE_WARNING=true node -e 'require("stylelint")'
(no output)

Why not use quietDeprecationWarnings

It's difficult to write a Rollup plugin code using the quietDeprecationWarnings option in this case. But I'm open to feedback for a better solution.

This change deprecates the use of CJS Node.js API and updates the migration guide to 16.0.0.

Stylelint will show the deprecation warning when using CJS API.
This deprecation code is generated by a Rollup custom plugin.

Examples:

```sh-session
$ node -e 'import("stylelint")'
(no output)

$ node -e 'require("stylelint")'
The CJS build of Stylelint's Node.js API is deprecated. See https://stylelint.io/migration-guide/to-16

$ STYLELINT_CJS_IGNORE_WARNING=true node -e 'require("stylelint")'
(no output)
```
Copy link

changeset-bot bot commented Nov 21, 2023

🦋 Changeset detected

Latest commit: 5305bbf

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
stylelint Major

Not sure what this means? Click here to learn what changesets are.

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

@ybiquitous ybiquitous marked this pull request as ready for review November 21, 2023 14:53
Copy link
Member

@jeddy3 jeddy3 left a comment

Choose a reason for hiding this comment

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

@ybiquitous Thank you for tackling this!

Injecting the deprecation warning into the CJS code using rollup is smart.

Is there a way to silence the warning as a CJS plugin author when using Jest?

console.warn
      The CJS build of Stylelint's Node.js API is deprecated. See https://stylelint.io/migration-guide/to-16

      17 |
      18 |      if (!options.quietDeprecationWarnings) {
    > 19 |              console.warn("The CJS build of Stylelint's Node.js API is deprecated. See https://stylelint.io/migration-guide/to-16");
         |                      ^
      20 |      }
      21 |
      22 |      const cwd = options.cwd || process.cwd();

      at createStylelint (../stylelint/lib/createStylelint.cjs:19:11)
      at standalone (../stylelint/lib/standalone.cjs:94:20)
      at Object.<anonymous> (node_modules/jest-preset-stylelint/getTestRule.js:43:21)

Or do we need to reintroduce the STYLELINT_CJS_IGNORE_WARNING method alongside quietDeprecationWarnings?

{
  "scripts": {
    "test": "STYLELINT_CJS_IGNORE_WARNING=true jest"
  }
}

lib/createStylelint.mjs Outdated Show resolved Hide resolved
@ybiquitous
Copy link
Member Author

Is there a way to silence the warning as a CJS plugin author when using Jest?

Ah, good catch. We also need to adjust jest-preset-stylelint toward ESM. 😅

At this time, using loadLint with dynamic import() seems good:

// jest.setup.js
global.testRule = getTestRule({ loadLint: () => import("stylelint").then(m => m.default.lint) });

// or a test file
testRule({
  loadLint: () => import("stylelint").then(m => m.default.lint),
  ruleName: 'block-no-empty',
  config: true,
  accept: [/*...*/],
  reject: [/*...*/],
});

Otherwise, changing the default loadLint function may be better:

- schema.loadLint || options.loadLint || (() => Promise.resolve(require('stylelint').lint));
+ schema.loadLint || options.loadLint || (() => import('stylelint').then(m => m.default.lint));

although this may be a breaking change.

@ybiquitous
Copy link
Member Author

@jeddy3 I've opened PR stylelint/jest-preset-stylelint#87 as I suggested on #7353 (comment). If this new jest-preset-stylelint would be available, the console.warn() warning should not be shown by default (since the ESM version would be loaded).

Copy link
Member

@Mouvedia Mouvedia left a comment

Choose a reason for hiding this comment

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

LGTM

docs/user-guide/node-api.md Show resolved Hide resolved
lib/__tests__/plugins.test.mjs Show resolved Hide resolved
Copy link
Member

@jeddy3 jeddy3 left a comment

Choose a reason for hiding this comment

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

@ybiquitous Thank you!

I've requested some minor copy changes.

(I was offline for the weekend and looping back to this PR now.)

rollup.config.mjs Outdated Show resolved Hide resolved
lib/augmentConfig.mjs Outdated Show resolved Hide resolved
docs/migration-guide/to-16.md Outdated Show resolved Hide resolved
Copy link
Member

@jeddy3 jeddy3 left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@ybiquitous ybiquitous changed the title Deprecate CJS Node.js API Deprecate CommonJS Node.js API Nov 27, 2023
@ybiquitous ybiquitous merged commit 40df51f into main Nov 27, 2023
16 checks passed
@ybiquitous ybiquitous deleted the issue-7340 branch November 27, 2023 06:35
@ybiquitous
Copy link
Member Author

Thanks for the review! 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Deprecate CommonJS plugins
3 participants