Skip to content
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(core): determine if a module is a builtin using module.isBuiltin #5997

Merged
merged 12 commits into from
Aug 25, 2024

Conversation

zouguangxian
Copy link
Contributor

@zouguangxian zouguangxian commented Dec 3, 2023

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

@zouguangxian 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 zouguangxian force-pushed the fix/builtin-modules branch 2 times, most recently from b765721 to d20823c Compare December 3, 2023 04:14
Copy link
Member

@merceyz merceyz left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@adrian-gierakowski
Copy link

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

#6135
#5417
#5637

@adrian-gierakowski
Copy link

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

@jkowalleck
Copy link
Contributor

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

@jkowalleck
Copy link
Contributor

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

fixes #6135
fixes #5417
fixes #5637

@jkowalleck
Copy link
Contributor

jkowalleck commented Jun 26, 2024

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.

github-merge-queue bot pushed a commit that referenced this pull request Aug 25, 2024
## What's the problem this PR addresses?

<!-- Describe the rationale of your PR. -->
<!-- Link all issues that it closes. (Closes/Resolves #xxxx.) -->

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?

<!-- A detailed description of your implementation. -->

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

<!--- Don't worry if you miss something, chores are automatically
tested. -->
<!--- This checklist exists to help you remember doing the chores when
you submit a PR. -->
<!--- Put an `x` in all the boxes that apply. -->
- [x] I have read the [Contributing
Guide](https://yarnpkg.com/advanced/contributing).

<!-- See
https://yarnpkg.com/advanced/contributing#preparing-your-pr-to-be-released
for more details. -->
<!-- Check with `yarn version check` and fix with `yarn version check
-i` -->
- [x] I have set the packages that need to be released for my changes to
be effective.

<!-- The "Testing chores" workflow validates that your PR follows our
guidelines. -->
<!-- If it doesn't pass, click on it to see details as to what your PR
might be missing. -->
- [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
Copy link
Contributor

jkowalleck commented Aug 25, 2024

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.

@merceyz 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
merceyz
merceyz previously approved these changes Aug 25, 2024
@merceyz merceyz enabled auto-merge August 25, 2024 13:06
@merceyz 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
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.

[Bug?]: Plugins don't work in 4.1.0 [Bug?]: node: protocol is not supported in plugin factory require
5 participants