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 false positive CJS deprecation warning for dual-package plugins #7532
Conversation
🦋 Changeset detectedLatest commit: 8911631 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
Also, I'm not sure why this PR could resolve #7460... |
I only enable This PR is not finally finished yet. |
Hum, this solution is not so good because the building becomes complex. 😕 |
@ybiquitous I'd like to skip building that |
Based on their names, |
@Mouvedia I don't think that helps for our situation.
If we don't want pure ESM dependency and don't want to keep |
I feel like it's better to unify stylelint/lib/utils/dynamicImport.mjs Line 11 in c49f9b5
|
Indeed, it seems we don't need (A failing case would be global plugins, should we support it for compatibility? |
Yes. I feel we can refactor |
It may be a quite big refactoring, though. |
@ybiquitous I have several solutions:
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 |
We have already used rollup. In addition, PR #7535 tries adding esbuild. I doubt if the way is a suitable solution for this problem.
I think non-breaking because |
So, this PR is simplest one to fix that issue.
Line 37 in c49f9b5
All utils are exported as public API. |
Not correct. The public API is exposed as |
So what is your preferred way to fix that issue considering the contribution cost? Notice: 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 And I don't quite understand, Whatever, I need to get what's the exact direction you want to move forward? |
Although 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
At the same time, we have to remove the following code: stylelint/lib/augmentConfig.mjs Lines 150 to 158 in c49f9b5
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...
} |
But I don't think we can support global plugins without And also, we have many dependencies already, |
@Mouvedia I believe Yes. And I think we don't need to care about that too much. |
I think this $ 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 In addition, I think using
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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?
Yes, it's a blocker. |
@ybiquitous This seems making sense to me. 🤔 @Mouvedia How do you think? And at the same time we need to catch |
When using |
Find better arguments to convince @ybiquitous :) 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. |
|
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;
}
} |
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"
}
} |
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 can give it a try if you want. |
Okay, let's keep it as-is.
Yes, please. |
@ybiquitous I just added the test case at |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JounQin Thanks for adding the test case. Putting node_modules
in the fixtures directory is a nice idea!
@JounQin I still have a concern about using |
Co-authored-by: Masafumi Koba <473530+ybiquitous@users.noreply.github.com>
I was thinking we don't need change at this PR. I'll change to use |
Sorry, this comment meant "try...catch", not |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your hard work! LGTM 👍🏼
fix #7460
close #7535
I don't know how to keeplib/utils/getModulePath.mjs
different withlib/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 originalimport-meta-resolve
instead. This task should be added into #7396 if this PR is merged.