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(plugin-pnp): unset NODE_OPTIONS when preparing the environment after switching linkers #5616

Merged
merged 4 commits into from
Jul 28, 2023

Conversation

paul-soporan
Copy link
Member

What's the problem this PR addresses?

We set NODE_OPTIONS for PnP in setup-ts-execution, which causes yarn config set nodeLinker && yarn install to fail due to NODE_OPTIONS still referencing the removed PnP files during build scripts.

I consider this a Yarn issue and not a setup-ts-execution issue because Yarn, like any other package, is supposed to work under PnP, even if NODE_OPTIONS happens to be set.

Fixes https://github.com/yarnpkg/berry/actions/runs/5669921008/job/15363692897#step:4:229.

How did you fix it?

This PR makes setupScriptEnvironment from @yarnpkg/plugin-pnp remove the PnP flags from NODE_OPTIONS if the linker isn't pnp.

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.

@paul-soporan
Copy link
Member Author

I also changed setup-ts-execution to use --require instead of -r, as we don't support the -r alias at the moment.

I've added a note about it and will fix it in a different PR, as I don't want to expand the scope of this one.

@paul-soporan
Copy link
Member Author

I also made the hook path unquoted in setup-ts-execution because changing the -r to --require highlighted a different issue - the fact that we don't support quoted paths in NODE_OPTIONS.

I opened a PR (#3232) in the past fixing this but abandoned / forgot about it because I discovered that it's impossible to modify NODE_OPTIONS via RegExp and without a proper argument parser and correctly support all edge cases.

I'll look into reviving that PR with a simpler solution that works most of the time - in the meantime I've added a workaround to this one.

@arcanis arcanis merged commit 1123f86 into master Jul 28, 2023
17 checks passed
@arcanis arcanis deleted the paul/fix/unset-node-options branch July 28, 2023 15:54
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.

2 participants