Skip to content

Conversation

@JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Oct 25, 2023

This is a breaking change.

We bumped the yarn dependencies and adapted to Yarn 4 breaking changes. Because the plugin relies on yarn internal packages, we will not support Yarn 3 in the near future. Use 0.1.0 if you are using Yarn 3.

The built plugin seems to work with the Babel repo after it upgraded to Yarn 4.

@@ -1,5 +1,7 @@
plugins:
- path: .yarn/plugins/@yarnpkg/plugin-interactive-tools.cjs
spec: "@yarnpkg/plugin-interactive-tools"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The plugin is removed because Yarn 4 includes this plugin.

@@ -1,2047 +0,0 @@
import { URL as URL$1, fileURLToPath, pathToFileURL } from 'url';
Copy link
Owner

Choose a reason for hiding this comment

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

Do we need to add this file to .gitignore (like .pnp.cjs), or does Yarn not generate it anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yarn 4 does not generate this file anymore, only .pnp.cjs is generated. Maybe it has been included in Yarn 4 as well.

Copy link

@merceyz merceyz Oct 28, 2023

Choose a reason for hiding this comment

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

Yarn generates it if it detects that ESM support is required, it should be added to .gitignore if you're not using zero-installs and PnP.

@JLHwung
Copy link
Contributor Author

JLHwung commented Oct 27, 2023

I will open a new PR to Babel using the built version of this PR, if the Babel CI is green it should be good to merge.

JLHwung added a commit to JLHwung/babel that referenced this pull request Oct 27, 2023
JLHwung added a commit to JLHwung/babel that referenced this pull request Oct 27, 2023
@JLHwung
Copy link
Contributor Author

JLHwung commented Oct 31, 2023

Currently the Babel CI is failing because the serialized esmExports info is unfortunately recognized as multiple cache keys in Yarn 4. I will try to move on to a new delimiter, maybe | or even a lone surrogate U+F800 if we ever want to fully support string module names.

@merceyz
Copy link

merceyz commented Oct 31, 2023

That RegExp hasn't changed for 4 years so that's odd, I would expect it to be related to yarnpkg/berry#4305.
Specifically I think opts.project.configuration.normalizeDependencyMap needs to be called on this Map:

dependencies: new Map(
[
consequent && [consequentDesc.identHash, consequentDesc],
alternate && [alternateDesc.identHash, alternateDesc],
].filter(Boolean) as [IdentHash, Descriptor][]
),
.

@JLHwung
Copy link
Contributor Author

JLHwung commented Oct 31, 2023

image

I am not familiar with the internals, maybe the CI error is due to the PR as you mentioned. In the debugger the key split is the first step where the descriptor range key2 "...(esm:default,Chalk)" is incorrectly cropped into entry "...(esm:default", which is later thrown by the condition plugin.

On a second thought, maybe that regex should have been / *, +/g? Since the generated lockfile always separate items by , .

I will try your approach, it would be great if we don't have to move on a new serialization. Thank you!

Update: It seems to me normalizeDependencyMap does not work, as the stringified range still contain , and will be split by the regex.

@nicolo-ribaudo
Copy link
Owner

Ty! Is this ready now that CI on the Babel PR is passing?

@JLHwung
Copy link
Contributor Author

JLHwung commented Nov 8, 2023

Ty! Is this ready now that CI on the Babel PR is passing?

Yes it is ready for review now. 🙏

compressionLevel: mixed

yarnPath: .yarn/releases/yarn-3.6.1.cjs
enableGlobalCache: false

Choose a reason for hiding this comment

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

Just curious, is there any reason for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is automatically generated by Yarn 4 when migrating from Yarn 3 config. The default is now true in Yarn 4, but it was false in Yarn 3.

@nicolo-ribaudo nicolo-ribaudo merged commit 3435122 into nicolo-ribaudo:main Nov 13, 2023
nicolo-ribaudo pushed a commit to JLHwung/babel that referenced this pull request Nov 13, 2023
nicolo-ribaudo pushed a commit to JLHwung/babel that referenced this pull request Nov 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants