fix(core): determine if a module is a builtin using `module.isBuiltin` #5997

zouguangxian · 2023-12-03T03:08:01Z

What's the problem this PR addresses?

builder plugin may output node:process instead of process in the generated file.
When executing yarn., it will report an error like:

This plugin cannot access the package referenced via node:process which is neither a builtin, nor an exposed entry

** Environment

❯ yarn tsc --version
Version 5.3.2

...

How did you fix it?

Checklist

I have read the Contributing Guide.

I have set the packages that need to be released for my changes to be effective.

I will check that all automated PR checks pass before the PR gets reviewed.

merceyz

Node.js exposes a module.isBuiltin function that can handle this for us, could you change the implementation to use that instead?

Please add a test to https://github.com/yarnpkg/berry/blob/f3775aaec22dd05c83b9a144a2c3aa52e8774c21/packages/acceptance-tests/pkg-tests-specs/sources/features/plugins.test.ts.

e.g. This plugin cannot access the package referenced via node:process which is neither a builtin, nor an exposed entry

zouguangxian · 2024-02-08T09:55:34Z

Node.js exposes a module.isBuiltin function that can handle this for us, could you change the implementation to use that instead?

Please add a test to https://github.com/yarnpkg/berry/blob/f3775aaec22dd05c83b9a144a2c3aa52e8774c21/packages/acceptance-tests/pkg-tests-specs/sources/features/plugins.test.ts.

Is it possible for you to figure out how to use module.isBuiltin to handle this? Is the current implementation the most simple?

zouguangxian · 2024-02-20T08:47:21Z

@merceyz Is it possible for you to review this PR?

adrian-gierakowski · 2024-04-25T14:02:21Z

would be great to have this merged as there are currently 3 open issues that this would fix:

#6135
#5417
#5637

adrian-gierakowski · 2024-04-25T14:46:59Z

@zouguangxian just wanted to say thank you as this patch works for me!

jkowalleck · 2024-06-26T08:33:10Z

@merceyz @arcanis @zouguangxian ,
could you maybe rebase or merge in latest master, and have the CI/CT run again?

jkowalleck · 2024-06-26T08:35:12Z

@zouguangxian could you add the following to the PR description?

fixes #6135
fixes #5417
fixes #5637

jkowalleck · 2024-06-26T10:59:49Z

the following is a backwards-compatible patch of the plugin-compiler: #6356.
That patched compiler is able to produce backwards-compatible plugins. Therefore, that patched plugin-compiler should enable plugins to be runnable without this very patched yarn-core.

## What's the problem this PR addresses?   Yarn plugins used to be forbidden to import/require built-in modules prefixed with `node:`. see #6135 see #5417 Fixes #5637 The yarn plugin builder should be aware of this fact and produce bundled code, that does not contain any `node:` prefixed import/require. This is especially important when building plugins with 3rd party dependencies, where the plugin author cannot "fix" the imports to yarn's needs. ## How did you fix it?  I enabled the plugin-compiler to generate the plugin-code as needed: I utilized the capability of `esbuild` to strip these `node:` prefixes from import/require instructions. Therefore, I added config options to the plugin build process to instruct `esbuild` to do so. This is a fix of the plugin builder, which enables plugin authors to compile their work in a backwards-compatible way, so that the build result is runnable in old/unpatched versions of yarn. Unpatched regarding #5997 ## Related The #5997 tries to address the issue from the plugin-runtime side. This would enable "broken" plugins to be runnable in all future/patched versions of yarn-core. ## Additionally This very PR aims to enable plugin authors to create plugins that are runnable with unpatched versions of yarn-core. It is considered a friction-free backwards-compatible solution on all ends. Yet it does not replace #5997. ## Checklist    - [x] I have read the [Contributing Guide](https://yarnpkg.com/advanced/contributing).   - [x] I have set the packages that need to be released for my changes to be effective.   - [x] I will check that all automated PR checks pass before the PR gets reviewed. --------- Signed-off-by: Jan Kowalleck <jan.kowalleck@gmail.com> Co-authored-by: Maël Nison <nison.mael@gmail.com>

jkowalleck · 2024-08-25T12:02:37Z

What's the problem this PR addresses?

builder plugin may output node:process instead of process in the generated file. When executing yarn., it will report an error like:

shoud be fixed via #6356
the next version of yarn plugin builder/bundler should include a feature that solves this behavior for you.

Anyway, #6356 does NOT obsolete this very pull request.

#6356 is a solution which requires plugin authors to upgrade their build environment and release a fixed version of their build result,
while this very PR is a solution which targets the actual plugin runtime, so existing false-negative "broken" plugins become working ones as expected.

zouguangxian changed the title ~~fix: support both of process and node:process~~ fix: Support both formats to reference builtin modules, such as process and node:process. Dec 3, 2023

zouguangxian force-pushed the fix/builtin-modules branch 2 times, most recently from b765721 to d20823c Compare December 3, 2023 04:14

merceyz requested changes Dec 3, 2023

View reviewed changes

zouguangxian added 2 commits February 8, 2024 15:57

fix: support both of process and node:process

58a7866

e.g. This plugin cannot access the package referenced via node:process which is neither a builtin, nor an exposed entry

build: update version configuration for the affected packages

e711ccc

zouguangxian force-pushed the fix/builtin-modules branch from d20823c to e711ccc Compare February 8, 2024 07:57

zouguangxian requested a review from merceyz February 8, 2024 09:55

test: require core modules with node:

d354ab5

zouguangxian force-pushed the fix/builtin-modules branch from 3b43ece to d354ab5 Compare February 8, 2024 09:56

Merge branch 'master' into fix/builtin-modules

4b82327

adrian-gierakowski mentioned this pull request Apr 25, 2024

[Bug?]: @yarnpkg/builder@4.0.0-rc.48 produces bundles which yarn cannot consume #5637

Closed

1 task

arcanis added 4 commits April 25, 2024 17:12

Update Configuration.ts

7cd908b

Merge branch 'master' into fix/builtin-modules

f643ded

Update Configuration.ts

0186ad0

Update plugins.test.ts

ff344bd

merceyz mentioned this pull request Jun 26, 2024

feat: allow modules prefixed with node: #6355

Closed

3 tasks

jkowalleck mentioned this pull request Jun 26, 2024

feat(builder): transform node: imports as needed #6356

Merged

3 tasks

merceyz added 2 commits August 25, 2024 14:58

Merge branch 'master' into fix/builtin-modules

eff9f81

chore: update versions

4b09860

merceyz changed the title ~~fix: Support both formats to reference builtin modules, such as process and node:process.~~ fix(core): determine if a module is a builtin using module.isBuiltin Aug 25, 2024

chore: lint

5db128a

This was linked to issues Aug 25, 2024

[Bug?]: Plugins don't work in 4.1.0 #6135

Closed

[Bug?]: node: protocol is not supported in plugin factory require #5417

Closed

merceyz previously approved these changes Aug 25, 2024

View reviewed changes

merceyz enabled auto-merge August 25, 2024 13:06

test: fix syntax error

85b3e2f

merceyz dismissed their stale review via 85b3e2f August 25, 2024 13:21

merceyz approved these changes Aug 25, 2024

View reviewed changes

merceyz added this pull request to the merge queue Aug 25, 2024

Merged via the queue into yarnpkg:master with commit 3b156c9 Aug 25, 2024
26 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(core): determine if a module is a builtin using `module.isBuiltin` #5997

fix(core): determine if a module is a builtin using `module.isBuiltin` #5997

zouguangxian commented Dec 3, 2023 •

edited

Loading

merceyz left a comment •

edited

Loading

zouguangxian commented Feb 8, 2024

zouguangxian commented Feb 20, 2024

adrian-gierakowski commented Apr 25, 2024

adrian-gierakowski commented Apr 25, 2024

jkowalleck commented Jun 26, 2024

jkowalleck commented Jun 26, 2024

jkowalleck commented Jun 26, 2024 •

edited

Loading

jkowalleck commented Aug 25, 2024 •

edited

Loading

fix(core): determine if a module is a builtin using module.isBuiltin #5997

fix(core): determine if a module is a builtin using module.isBuiltin #5997

Conversation

zouguangxian commented Dec 3, 2023 • edited Loading

merceyz left a comment • edited Loading

Choose a reason for hiding this comment

zouguangxian commented Feb 8, 2024

zouguangxian commented Feb 20, 2024

adrian-gierakowski commented Apr 25, 2024

adrian-gierakowski commented Apr 25, 2024

jkowalleck commented Jun 26, 2024

jkowalleck commented Jun 26, 2024

jkowalleck commented Jun 26, 2024 • edited Loading

jkowalleck commented Aug 25, 2024 • edited Loading

fix(core): determine if a module is a builtin using `module.isBuiltin` #5997

fix(core): determine if a module is a builtin using `module.isBuiltin` #5997

zouguangxian commented Dec 3, 2023 •

edited

Loading

merceyz left a comment •

edited

Loading

jkowalleck commented Jun 26, 2024 •

edited

Loading

jkowalleck commented Aug 25, 2024 •

edited

Loading