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-pack): reload manifest after prepack script #3146

Merged
merged 2 commits into from
Aug 17, 2021

Conversation

kherock
Copy link
Contributor

@kherock kherock commented Jul 20, 2021

What's the problem this PR addresses?

Resolves #3145 - see issue for rationale.

How did you fix it?

This unconditionally reloads the active workspace's manifest during prepareForPack after the prepack lifecycle script exits.

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.

@kherock kherock force-pushed the pack-reload-manifest branch from 8abe25a to ce14a28 Compare July 20, 2021 14:36
Copy link
Member

@arcanis arcanis left a comment

Choose a reason for hiding this comment

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

That won't work. We special-case package.json precisely because we want to use the manifest from memory, because beforeWorkspacePacking is meant to allow in-memory package mutations (which we use to implement publishConfig).

Generally, prepack is intended to be idempotent, so mutating the package.json isn't a practice we'd recommend. Instead, you should use beforeWorkspacePacking (which is more verbose). It's a bit unclear by your issue what you mean by "I want to Yarn to still perform the processing it normally does", so I'm not sure why it wouldn't work.

@kherock
Copy link
Contributor Author

kherock commented Jul 20, 2021

Just to be clear, this isn't at odds with what beforeWorkspacePacking does. I also wouldn't advise mutating package.json, but I think disallowing it altogether is unfair especially since this flow works just fine with npm (and I believe Yarn classic). I'm viewing this as an escape hatch when publishConfig isn't enough.

It's a bit unclear by your issue what you mean by "I want to Yarn to still perform the processing it normally does"

I want Yarn and its plugins to still run their beforeWorkspacePacking after the updated package.json is loaded. There's the built-in one that validates and replaces workspace: protocols, but I also don't want to interfere with any other hooks. If I were to write my own beforeWorkspacePacking, I'd need a way to ensure that my hook runs first.

@kherock
Copy link
Contributor Author

kherock commented Aug 4, 2021

Any further thoughts on this? I really can't see how supporting this would disrupt anything. I've been using this patch in my CI/CD pipeline for the past few weeks, and I actually already do use publishConfig in addition to some prepack scripts that do further preprocessing work.

@kherock
Copy link
Contributor Author

kherock commented Aug 12, 2021

@arcanis @merceyz Just bumping this one last time for a second opinion. I'm not really sure how else to really solve this short of patching the Yarn CLI with every release 😕. Intuitively, I'd expect that yarn run prepack && yarn pack would generate the same archive as a single run of yarn pack.

@kherock kherock force-pushed the pack-reload-manifest branch from ce14a28 to ee89a7c Compare August 12, 2021 13:53
@arcanis
Copy link
Member

arcanis commented Aug 12, 2021

Yep, that looks good to me - I've updated the versions since I think it qualifies as a bugfix more than a breaking change.

@arcanis arcanis enabled auto-merge (squash) August 12, 2021 14:34
arcanis
arcanis previously approved these changes Aug 12, 2021
@arcanis arcanis merged commit c8afad3 into yarnpkg:master Aug 17, 2021
@merceyz merceyz changed the title feat(plugin-pack): reload manifest after prepack script fix(plugin-pack): reload manifest after prepack script Aug 17, 2021
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.

[Feature] Allow prepack lifecycle script to do preprocessing on package.json
2 participants