🦋 Changeset detected

Latest commit: 8911631

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

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

@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.

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

@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.

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. 😕

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

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

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

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

1. We need to resolve module path instead of only import.meta.url
2. 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.

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

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

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

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?

It may be a quite big refactoring, though.

@ybiquitous I have several solutions:

1. Use different solution for getModulePath.mjs vs getModulePath.cjs like this PR
2. bundle import-meta-resolve into cjs with esbuild (see fix: replace resolve-from with import-meta-resolve for dual package #7535)
3. use import('import-meta-resolve') + https://github.com/un-ts/synckit, no internal API change
4. change getModulePath's API to async
5. 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?

@ybiquitous Any news?

@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.

@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.

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?

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?

All utils are exported as public API.

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

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

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.

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?

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:

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

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...
}

@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.