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: don't write pnpapi binary into own pkg #2457

Merged
merged 3 commits into from
Aug 29, 2022
Merged

Conversation

jonaskuske
Copy link
Contributor

583569e changed the PnP workaround so that the binary is copied into the esbuild package location instead of a global cache dir. However, packages should never write into their own folder as it (usually) is read-only: https://yarnpkg.com/advanced/rulebook/#packages-should-never-write-inside-their-own-folder-outside-of-postinstall

In most cases this worked because esbuild is automatically unplugged due to its postinstall script and therefore its folder becomes writeable. However, as soon as you disable scripts for the package or globally in .yarnrc.yml, esbuild won't be unplugged any longer, its folder becomes read-only and everything breaks. So instead, this PR writes the PnP binary to node_modules/.esbuild/<version>/.

@jonaskuske
Copy link
Contributor Author

Used a dotfolder inside node_modules as that's what most tools are doing, but any location is fine really.
On that note: my PR is pretty similar to the fix committed immediately before the location was switched to esbuild's own folder: 98b0640#diff-c3519ab69ed278185e3afc76a8c10dcecad25c041002933b708fff3a064c8608R105-R114. Any reason the mkdirSync solution wasn't enough and the binary location was changed one commit later?

@evanw
Copy link
Owner

evanw commented Aug 29, 2022

Thanks for the proposed fix.

Any reason the mkdirSync solution wasn't enough and the binary location was changed one commit later?

IIRC I removed the cache logic in 583569e both because it has been a source some of maintenance overhead (cache locations outside of the project directory sometimes don't work for certain people) and because caching the binary inside the project means the memory space is automatically reclaimed when the project is directory deleted, which was not previously the case.

@evanw
Copy link
Owner

evanw commented Aug 29, 2022

Just looked into this more. This should have been caught by CI here:

esbuild/Makefile

Lines 181 to 183 in 7cae769

# Test install without scripts
rm -fr e2e-yb && mkdir e2e-yb && cd e2e-yb && echo {} > package.json && echo 'enableScripts: false' > yarn.lock && yarn set version berry && yarn add esbuild
cd e2e-yb && echo "1+2" | yarn esbuild && yarn node -p "require('esbuild').transformSync('1+2').code"

Unfortunately I don't use Yarn myself and so I ended up configuring Yarn incorrectly when writing the test. I'll fix that as well.

@evanw evanw merged commit 4eb1713 into evanw:master Aug 29, 2022
evanw added a commit that referenced this pull request Aug 29, 2022
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