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

Fix handling of plugins that are hybrid CommonJS and ESM packages #7460

Closed
henryruhs opened this issue Jan 12, 2024 · 25 comments · Fixed by #7532
Closed

Fix handling of plugins that are hybrid CommonJS and ESM packages #7460

henryruhs opened this issue Jan 12, 2024 · 25 comments · Fixed by #7532
Labels
status: wip is being worked on by someone type: bug a problem with a feature or rule

Comments

@henryruhs
Copy link
Contributor

henryruhs commented Jan 12, 2024

What minimal example or steps are needed to reproduce the bug?

The stylelint plugin that I build with rollup outputs a index.cjs and index.mjs file.

In package.json file:

"main": "index.cjs",
"module": "index.mjs"

Package structure

grafik

What did you expect to happen?

Stylelint 15 takes the cjs file and Stylelint 16 the mjs file

What actually happened?

CommonJS plugins are deprecated ("/home/.../.../node_modules/@isnotdefined/stylelint-plugin/index.cjs")

Related issue: isnotdefinedcom/stylelint-plugin#14

@JounQin
Copy link
Member

JounQin commented Jan 12, 2024

I'll take a look on this.

@ybiquitous ybiquitous added the status: needs investigation triage needs further investigation label Jan 14, 2024
@jeddy3 jeddy3 changed the title Bad handling of plugins that provide CommonJS and ESM Fix handling of plugins that are hybrid CommonJS and ESM packages Jan 20, 2024
@henryruhs
Copy link
Contributor Author

Updates on this?

@ybiquitous
Copy link
Member

@henryruhs Can you provide more details on the problem in your plugin? What do you expect?

@JounQin
Copy link
Member

JounQin commented Feb 20, 2024

module is not a standard property for ESM, you need to use exports instead.

@henryruhs
Copy link
Contributor Author

henryruhs commented Feb 20, 2024

From my knowledge, the hybrid approach that I picked is valid. Stylelint ignores the ESM module entry point.

I'm probably too much of a TypeScript person and remember that "exports" are not supported.

Edit: will update to exports.

@henryruhs henryruhs reopened this Feb 20, 2024
@henryruhs
Copy link
Contributor Author

henryruhs commented Feb 20, 2024

@JounQin I reopen the issue... this is loading the common.js version again.

"exports":
{
	"require": "index.cjs",
	"import": "index.js"
}

There is a problem on your side... I give up on sorting this out and now will export ESM only for my plugin.

@JounQin
Copy link
Member

JounQin commented Feb 20, 2024

@henryruhs

Thanks for your investigation, I'm taking a look.

@henryruhs
Copy link
Contributor Author

@JounQin You can used my failed approach: https://www.npmjs.com/package/@isnotdefined/stylelint-plugin/v/4.0.0-fix.1

@JounQin
Copy link
Member

JounQin commented Feb 20, 2024

image image

I find another issue...

@JounQin
Copy link
Member

JounQin commented Feb 20, 2024

image

@henryruhs OK

The exports should be

{
  "exports": {
    "require": "./index.cjs",
    "import": "./index.js"
  }
}

After fixing the ./ issue, CommonJS plugins are deprecated warning issue is now reproduced.

@henryruhs
Copy link
Contributor Author

There is no need for the ./ as the files are in the root.

The code of 4.0.0-fix.1 looked exact like you suggested: isnotdefinedcom/stylelint-plugin@76febb3

@JounQin
Copy link
Member

JounQin commented Feb 20, 2024

@henryruhs

There is no need for the ./ as the files are in the root.

No, ./ is required in exports AFAIK.


The issue here is, resolve-from is a plain commonjs package, so it will resolve hybrid packages as commonjs, what means commonjs is preferred over ESM. I'll find better solution.

@henryruhs
Copy link
Contributor Author

I disagree... the problem is not resolving the path - it's selecting the wrong package.

@JounQin
Copy link
Member

JounQin commented Feb 20, 2024

I disagree... the problem is not resolving the path - it's selecting the wrong package.

Did you see my comments?

After fixing the ./ issue, CommonJS plugins are deprecated warning issue is now reproduced.

The issue here is, resolve-from is a plain commonjs package, so it will resolve hybrid packages as commonjs, what means commonjs is preferred over ESM. I'll find better solution.

@JounQin JounQin added type: bug a problem with a feature or rule upstream relates to an upstream package and removed status: needs investigation triage needs further investigation labels Feb 20, 2024
@henryruhs
Copy link
Contributor Author

henryruhs commented Feb 20, 2024

Don't use packages by this guy, he created thousands of them and there is no chance he can maintain all of them.

@ybiquitous
Copy link
Member

@henryruhs Just to be sure, @isnotdefined/stylelint-plugin@5.0.0 has resolved this problem, right?

@JounQin
Copy link
Member

JounQin commented Feb 21, 2024

@ybiquitous That only resolves on the plugin's side with pure ESM only support, hybrid package will still warn about the deprecation message.

@henryruhs
Copy link
Contributor Author

henryruhs commented Feb 21, 2024

5.0.0 did not actually resolve it, just limited it to ESM to make it work.

@ybiquitous
Copy link
Member

Can you provide a reproducible demo/code with @isnotdefined/stylelint-plugin@5.0.0?

@ybiquitous ybiquitous removed the upstream relates to an upstream package label Feb 21, 2024
@ybiquitous ybiquitous added the status: wip is being worked on by someone label Feb 21, 2024
@henryruhs
Copy link
Contributor Author

There is this tiny Angular playground that I just updated via my smartphone.

It uses my stylelint config which loads the stylelint plugin.

https://github.com/henryruhs/ngx-crud-playground

@JounQin
Copy link
Member

JounQin commented Feb 21, 2024

Notice, 4.0.0-fix.1 is required for reproduction, 5.0.0 is an ESM only package now.

@ybiquitous
Copy link
Member

ybiquitous commented Feb 21, 2024

Thanks. I can now reproduce the deprecation warning with the patched @isnotdefined/stylelint-plugin@4.0.0-fix.1 👍🏼

--- node_modules/@isnotdefined/stylelint-plugin/package.json.orig	2024-02-21 13:20:26
+++ node_modules/@isnotdefined/stylelint-plugin/package.json	2024-02-21 13:19:57
@@ -62,7 +62,7 @@
 	},
 	"exports":
 	{
-		"require": "index.cjs",
-		"import": "index.js"
+		"require": "./index.cjs",
+		"import": "./index.js"
 	}
 }

package.json:

{
  "dependencies": {
    "@isnotdefined/stylelint-plugin": "4.0.0-fix.1",
    "stylelint": "16.2.1"
  }
}

.stylelintrc.json:

{
  "plugins": ["@isnotdefined/stylelint-plugin"],
  "rules": {"@isnotdefined/no-disable": true}
}

Run:

$ echo 'a{}' | npx stylelint
CommonJS plugins are deprecated ("/Users/masafumi.koba/tmp/foo/node_modules/@isnotdefined/stylelint-plugin/index.cjs"). See https://stylelint.io/migration-guide/to-16

So, I understand the problem that resolve-from returns a CJS file of the package, e.g.

$ node -pe "require('resolve-from').silent(process.cwd(), '@isnotdefined/stylelint-plugin')"
/Users/masafumi.koba/tmp/foo/node_modules/@isnotdefined/stylelint-plugin/index.cjs

@JounQin
Copy link
Member

JounQin commented Feb 21, 2024

Yes, that's what I said at #7460 (comment)

@jpzwarte
Copy link

jpzwarte commented Mar 5, 2024

FYI: this happens with the stylelint-use-logical package which supports both CJS & ESM:

CommonJS plugins are deprecated (".../node_modules/stylelint-use-logical/index.cjs"). See https://stylelint.io/migration-guide/to-16

Using stylelint-use-logical@2.1.2 and stylelint@16.2.1

See csstools/stylelint-use-logical#25

@JounQin
Copy link
Member

JounQin commented Mar 6, 2024

@jpzwarte #7532 will fix this issue.

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: bug a problem with a feature or rule
4 participants