Fix false positive CJS deprecation warning for dual-package plugins #7532

JounQin · 2024-02-20T11:44:53Z

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

Is there anything in the PR that needs further explanation?

~~I don't know how to keep lib/utils/getModulePath.mjs different with lib/utils/getModulePath.cjs because they should use different strategies.~~

This PR currently uses @dual-bundle/import-meta-resolve, when we work on next major release which should be ESM only, we should change to use original import-meta-resolve instead. This task should be added into #7396 if this PR is merged.

changeset-bot · 2024-02-20T11:44:56Z

🦋 Changeset detected

Latest commit: 8911631

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

This PR includes changesets to release 1 package

Name	Type
stylelint	Patch

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

rollup.config.mjs

ybiquitous · 2024-02-21T00:06:15Z

@JounQin import-meta-resolve is a pure ESM package, right? If so, we cannot use it because a CommonJS module cannot load a pure ESM package.

ybiquitous · 2024-02-21T00:07:05Z

Also, I'm not sure why this PR could resolve #7460...

JounQin · 2024-02-21T00:46:31Z

@ybiquitous

#7460 (comment)

I only enable import-meta-resolve for .mjs entry, for .cjs it still uses resolve-from that's why commonjs entry is resolved over ESM one previously.

This PR is not finally finished yet.

ybiquitous · 2024-02-21T00:59:35Z

I only enable import-meta-resolve for .mjs entry, for .cjs it still uses resolve-from that's why commonjs entry is resolved over ESM one previously.

Hum, this solution is not so good because the building becomes complex. 😕

JounQin · 2024-02-21T01:08:36Z

@ybiquitous I'd like to skip building that .mjs file just like cli.mjs.

Mouvedia · 2024-02-21T03:57:18Z

Based on their names, resolveImportMeta and import.meta.resolve() might be useful.

JounQin · 2024-02-21T04:41:02Z

Based on their names, resolveImportMeta and import.meta.resolve() might be useful.

@Mouvedia I don't think that helps for our situation.

We need to resolve module path instead of only import.meta.url
import.meta.resolve is not supported for all node versions we're targeting, that's why I'm using https://github.com/wooorm/import-meta-resolve ponyfill.

If we don't want pure ESM dependency and don't want to keep lib/utils/getModulePath.cjs specially ignored, we can bundle import-meta-resolve for commonjs.

ybiquitous · 2024-02-21T06:49:08Z

I feel like it's better to unify getModulePath() and dynamicImport() than this resolve-from and import-meta-resolve approach.

stylelint/lib/utils/dynamicImport.mjs

Line 11 in c49f9b5

export default function dynamicImport(path) {

JounQin · 2024-02-21T07:25:15Z

Indeed, it seems we don't need getModulePath at all, right?

(A failing case would be global plugins, should we support it for compatibility?

ybiquitous · 2024-02-21T08:23:51Z

Indeed, it seems we don't need getModulePath at all, right?

Yes. I feel we can refactor getModulePath() and dynamicImport(). The dynamic import() can load both ESM and CJS packages or files, right?

ybiquitous · 2024-02-21T08:25:19Z

It may be a quite big refactoring, though.

JounQin · 2024-02-22T11:46:00Z

@ybiquitous I have several solutions:

Use different solution for getModulePath.mjs vs getModulePath.cjs like this PR
bundle import-meta-resolve into cjs with esbuild (see fix: replace resolve-from with import-meta-resolve for dual package #7535)
use import('import-meta-resolve') + https://github.com/un-ts/synckit, no internal API change
change getModulePath's API to async
drop getModulePath and refactor to use dynamicImport

4 and 5 could be a huge task and may cause breaking change itself, and we're very likely to drop commonjs support in the next major version, I don't know is it worth.

I personally want to try option 2.

WDYT?

JounQin · 2024-02-23T18:56:42Z

@ybiquitous Any news?

ybiquitous · 2024-02-26T06:17:08Z

@JounQin Honestly, I don't want to use two build tools (rollup and esbuild) at the same time because of the dependency management and more complex build flow, which will no longer be necessary in the next major version.

JounQin · 2024-02-26T06:43:34Z

@ybiquitous I don't quite follow, so do you mean that issue should be marked as known issue? Then in next major release we'll migrate to pure ESM.

And this PR doesn't add rollup, it just adds a new partial of script.

ybiquitous · 2024-02-26T06:52:45Z

We have already used rollup. In addition, PR #7535 tries adding esbuild. I doubt if the way is a suitable solution for this problem.

4 and 5 could be a huge task and may cause breaking change itself

I think non-breaking because getModulePath and dynamicImport are a private utility, but do I miss anything?

JounQin · 2024-02-26T07:29:49Z

We have already used rollup

So, this PR is simplest one to fix that issue.

I think non-breaking because getModulePath and dynamicImport are a private utility, but do I miss anything?

stylelint/package.json

Line 37 in c49f9b5

"./lib/utils/*": "./lib/utils/*"

All utils are exported as public API.

ybiquitous · 2024-02-26T07:43:22Z

All utils are exported as public API.

Not correct. The public API is exposed as stylelint.utils only. The ./lib/utils/* files may be accessible but unsupported; it does not mean "public".

https://github.com/stylelint/stylelint/blob/c49f9b5a7b45cb35c58b5bc1cd669733896d0183/docs/developer-guide/plugins.md#stylelintutils

JounQin · 2024-02-26T08:01:17Z

So what is your preferred way to fix that issue considering the contribution cost?

Notice: getModulePath is used in many places, so I think option 5 will not work. Option 4 will change many internal or some stylelint.utils API into async. (I haven't confirmed it)

https://github.com/search?q=repo%3Astylelint%2Fstylelint%20getModulePath&type=code

ybiquitous · 2024-02-26T08:39:46Z

I prefer a way not to increase dependencies and not to bring a more complex build flow as much as possible. They often would produce maintenance problems.

JounQin · 2024-02-26T09:08:29Z

Did you mean you don't want import-meta-resolve? Then maybe try/catch + dynamicImport?

And I don't quite understand, resolve-from is very outdated for ESM, import-meta-resolve is very well maintained, why replacing the dependency is not fine?

Whatever, I need to get what's the exact direction you want to move forward?

ybiquitous · 2024-02-26T09:22:18Z

Although import-meta-resolve is well maintained, but it would increase an extra dependency and loading time, right? (because it includes many polyfill codes) If so, I do want to avoid adding dependencies.

To minimize potential side effects, I think we can fix the following code block, which loads a plugin function from a string:

stylelint/lib/augmentConfig.mjs

Lines 320 to 329 in c49f9b5

    
           if (typeof pluginLookup === 'string') { 
        
           	pluginImport = await dynamicImport(pluginLookup); 
        
           	// NOTE: This '.cjs' check is limited. Some CommonJS plugins may have the '.js' extension. 
        
           	if (!quietDeprecationWarnings && pluginLookup.endsWith('.cjs')) { 
        
           		console.warn( 
        
           			`CommonJS plugins are deprecated ("${pluginLookup}"). See https://stylelint.io/migration-guide/to-16`, 
        
           		); 
        
           	} 
        
           } else {

At the same time, we have to remove the following code:

stylelint/lib/augmentConfig.mjs

Lines 150 to 158 in c49f9b5

    
           if (config.plugins) { 
        
           	config.plugins = [config.plugins].flat().map((lookup) => { 
        
           		if (typeof lookup === 'string') { 
        
           			return getModulePath(configDir, lookup, cwd); 
        
           		} 
        
           		return lookup; 
        
           	}); 
        
           }

I imagine the updated code would be like this:

if (typeof pluginLookup === 'string') {
-	pluginImport = await dynamicImport(pluginLookup);
+	pluginImport = await importPlugin(pluginLookup);
}

// ...

async function importPlugin(lookup) {
	// Using dynamicImport()...

	// If dynamicImport() fails, try reading a file...
}

JounQin · 2024-02-27T17:58:30Z

@ybiquitous

But I don't think we can support global plugins without import-meta-resolve? import() only support local dependence by default, we need to resolve global plugins manually first. And resolve-global doesn't support ESM only package well.

And also, we have many dependencies already, resolve-from is also as slow import-meta-resolve/native import.meta.resolve, I still don't get the point. Correctness is more important than performance trade off IMO.

JounQin · 2024-03-07T12:21:23Z

@Mouvedia I believe Yes. And I think we don't need to care about that too much. resolve-from can also possibly throw critical error, but we didn't care previously, we just ignored such cases.

ybiquitous · 2024-03-07T12:21:48Z

I think this fs.existsSync check makes sense because import.meta.resolve returns a URL even if the given path doesn't exist. See the example below. Although ./lib/foo.js doesn't exist, the resolved URL is printed:

$ cat a.mjs
console.log(import.meta.resolve('./lib/foo.js'));

$ node a.mjs
file:///Users/masafumi.koba/git/stylelint/stylelint/lib/foo.js

$ ls lib/foo.js
ls: lib/foo.js: No such file or directory

$ node -v
v21.6.2

So, if import-meta-resolve raises an error, it should be a critical case that we shouldn't catch. For example, import-meta-resolve doesn't raise an error of ERR_MODULE_NOT_FOUND.

In addition, I think using resolve instead of the low-level moduleResolve makes sense because:

moduleResolve is discouraged by the import-meta-resolve author on unexpected ERR_PACKAGE_PATH_NOT_EXPORTED thrown wooorm/import-meta-resolve#24 (comment).
We will switch from import-meta-resolve (ponyfill) to import.meta.resolve someday. Then, resolve can be migrated more easily than moduleResolve.

Mouvedia

@Mouvedia fixed in bebcc68 (#7532)

This is indeed resolved.
@ybiquitous is this ready to be approved? Or the switch from moduleResolve to resolve a blocker?

ybiquitous · 2024-03-07T12:37:01Z

Or the switch from moduleResolve to resolve a blocker?

Yes, it's a blocker.

JounQin · 2024-03-07T12:38:12Z

We will switch from import-meta-resolve (ponyfill) to import.meta.resolve someday. Then, resolve can be migrated more easily than moduleResolve.

@ybiquitous This seems making sense to me. 🤔

@Mouvedia How do you think?

And at the same time we need to catch resolve's error silently for Yarn P'n'P or custom resolver actually.

ybiquitous · 2024-03-07T12:45:32Z

And at the same time we need to catch resolve's error silently for Yarn P'n'P or custom resolver actually.

When using resolve with Yarn P'n'P, what error is raised?

Mouvedia · 2024-03-07T12:49:10Z

@Mouvedia How do you think? [sic]

Find better arguments to convince @ybiquitous :)
For me the bug seems fixed, hence the rest is minor and can be resolved in a separate refactor PR.
Now if there are cases not covered by @ybiquitous's MRE I would like either to know them or for you to test them.

Being cautious was the right call; we can release a new version once the question on whether the refactor is necessary or not is settled.

JounQin · 2024-03-07T12:55:54Z

When using resolve with Yarn P'n'P, what error is raised?

@ybiquitous

wooorm/import-meta-resolve#10

ybiquitous · 2024-03-07T12:59:51Z

ERR_MODULE_NOT_FOUND is raised, right? If so, it seems that we can write like this:

try {
  // resolve(...)
} catch (e) {
  if (e.code === 'ERR_MODULE_NOT_FOUND') {
    // Ignore the error for Yarn P'n'P. See https://github.com/wooorm/import-meta-resolve/issues/10
  } else {
    throw e;
  }
}

ybiquitous · 2024-03-07T13:04:09Z

I was also wondering if we could add a test case to reproduce this issue. 🤔

E.g.

// packages/dual-package/package.json
{
  "name": "@stylelint/dual-package",
  "version": "1.0.0",
  "exports": {
    "require": "./index.cjs",
    "import": "./index.mjs"
  }
}

// package.json
{
  "devDependencies": {
    "@stylelint/dual-package": "file:packages/dual-package"
  }
}

JounQin · 2024-03-07T13:06:33Z

I don't quite get why we need to care about the error in a silent resolving function, but if you stick to it, I'm fine with that. However, as I understand, there could be still other undefinded/unknown errors could be thrown or even in tbe further.

I was also wondering if we could add a test case to reproduce this issue. 🤔

I can give it a try if you want.

ybiquitous · 2024-03-07T23:56:17Z

However, as I understand, there could be still other undefinded/unknown errors could be thrown or even in tbe further.

Okay, let's keep it as-is.

I was also wondering if we could add a test case to reproduce this issue. 🤔
I can give it a try if you want.

Yes, please.

JounQin · 2024-03-08T06:26:50Z

@ybiquitous I just added the test case at a836e90 (#7532)

ybiquitous

@JounQin Thanks for adding the test case. Putting node_modules in the fixtures directory is a nice idea!

lib/__tests__/resolveSilent.test.mjs

ybiquitous · 2024-03-08T07:27:33Z

@JounQin I still have a concern about using moduleResolve instead of resolve as I commented on #7532 (comment). Don't your idea change?

Co-authored-by: Masafumi Koba <473530+ybiquitous@users.noreply.github.com>

JounQin · 2024-03-08T07:32:55Z

@JounQin I still have a concern about using moduleResolve instead of resolve as I commented on #7532 (comment). Don't your idea change?

Okay, let's keep it as-is.

I was thinking we don't need change at this PR.

I'll change to use resolve then.

ybiquitous · 2024-03-08T07:37:54Z

Okay, let's keep it as-is.

I was thinking we don't need change at this PR.

Sorry, this comment meant "try...catch", not moduleResolve. 🙏🏼

lib/__tests__/resolveSilent.test.mjs

.gitignore

ybiquitous

Thank you for your hard work! LGTM 👍🏼

JounQin requested review from ybiquitous and jeddy3 February 20, 2024 11:54

JounQin commented Feb 20, 2024

View reviewed changes

rollup.config.mjs Outdated Show resolved Hide resolved

JounQin mentioned this pull request Feb 22, 2024

Decide how to publish CJS bundles es-tooling/cjs-bundle#1

Open

JounQin mentioned this pull request Feb 22, 2024

fix: replace resolve-from with import-meta-resolve for dual package #7535

Closed

JounQin force-pushed the pkg_resolve branch from d9e9270 to 84f7990 Compare February 22, 2024 16:36

Mouvedia approved these changes Mar 7, 2024

View reviewed changes

test: add resolveSilent test case

a836e90

ybiquitous reviewed Mar 8, 2024

View reviewed changes

lib/__tests__/resolveSilent.test.mjs Outdated Show resolved Hide resolved

lib/__tests__/resolveSilent.test.mjs Outdated Show resolved Hide resolved

Apply suggestions from code review

a60dbec

Co-authored-by: Masafumi Koba <473530+ybiquitous@users.noreply.github.com>

refactor: change to use resolve instead of moduleResolve

9b86300

ybiquitous reviewed Mar 8, 2024

View reviewed changes

lib/__tests__/resolveSilent.test.mjs Outdated Show resolved Hide resolved

chore: move resolveSilent test case

8911631

Mouvedia reviewed Mar 8, 2024

View reviewed changes

.gitignore Show resolved Hide resolved

ybiquitous approved these changes Mar 8, 2024

View reviewed changes

ybiquitous merged commit fc88e99 into main Mar 8, 2024
14 checks passed

ybiquitous deleted the pkg_resolve branch March 8, 2024 22:25

ybiquitous mentioned this pull request Mar 11, 2024

Replace @dual-bundle/import-meta-resolve with import-meta-resolve #7556

Open

dflynn15 mentioned this pull request Mar 21, 2024

CommonJS plugins are deprecated csstools/stylelint-use-logical#27

Closed

Mouvedia mentioned this pull request Mar 25, 2024

Compatibility issue between stylelint@16.3.0 and stylelint-prettier@5.0.0 #7576

Closed

ybiquitous mentioned this pull request Mar 26, 2024

Fix import errors for configs and plugins omitting /index.js #7578

Merged

Fix false positive CJS deprecation warning for dual-package plugins #7532

Fix false positive CJS deprecation warning for dual-package plugins #7532

Conversation

JounQin commented Feb 20, 2024 • edited

changeset-bot bot commented Feb 20, 2024 • edited

🦋 Changeset detected

ybiquitous commented Feb 21, 2024

ybiquitous commented Feb 21, 2024

JounQin commented Feb 21, 2024

ybiquitous commented Feb 21, 2024

JounQin commented Feb 21, 2024

Mouvedia commented Feb 21, 2024 • edited

JounQin commented Feb 21, 2024 • edited

ybiquitous commented Feb 21, 2024

JounQin commented Feb 21, 2024 • edited

ybiquitous commented Feb 21, 2024

ybiquitous commented Feb 21, 2024

JounQin commented Feb 22, 2024 • edited

JounQin commented Feb 23, 2024

ybiquitous commented Feb 26, 2024 • edited

JounQin commented Feb 26, 2024 • edited

ybiquitous commented Feb 26, 2024

JounQin commented Feb 26, 2024

ybiquitous commented Feb 26, 2024

JounQin commented Feb 26, 2024 • edited

ybiquitous commented Feb 26, 2024

JounQin commented Feb 26, 2024 • edited

ybiquitous commented Feb 26, 2024

JounQin commented Feb 27, 2024

JounQin commented Mar 7, 2024 • edited

ybiquitous commented Mar 7, 2024

Mouvedia left a comment • edited

Choose a reason for hiding this comment

ybiquitous commented Mar 7, 2024

JounQin commented Mar 7, 2024 • edited

ybiquitous commented Mar 7, 2024

Mouvedia commented Mar 7, 2024

JounQin commented Mar 7, 2024 • edited

ybiquitous commented Mar 7, 2024 • edited

ybiquitous commented Mar 7, 2024

JounQin commented Mar 7, 2024 • edited

ybiquitous commented Mar 7, 2024

JounQin commented Mar 8, 2024

ybiquitous left a comment

Choose a reason for hiding this comment

ybiquitous commented Mar 8, 2024

JounQin commented Mar 8, 2024

ybiquitous commented Mar 8, 2024

ybiquitous left a comment

Choose a reason for hiding this comment

JounQin commented Feb 20, 2024 •

edited

changeset-bot bot commented Feb 20, 2024 •

edited

Mouvedia commented Feb 21, 2024 •

edited

JounQin commented Feb 21, 2024 •

edited

JounQin commented Feb 21, 2024 •

edited

JounQin commented Feb 22, 2024 •

edited

ybiquitous commented Feb 26, 2024 •

edited

JounQin commented Feb 26, 2024 •

edited

JounQin commented Feb 26, 2024 •

edited

JounQin commented Feb 26, 2024 •

edited

JounQin commented Mar 7, 2024 •

edited

Mouvedia left a comment •

edited

JounQin commented Mar 7, 2024 •

edited

JounQin commented Mar 7, 2024 •

edited

ybiquitous commented Mar 7, 2024 •

edited

JounQin commented Mar 7, 2024 •

edited