Skip to content

fix(core): fix the value check for publishConfig.provenance #6781

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

smorimoto
Copy link
Contributor

@smorimoto smorimoto commented May 2, 2025

What's the problem this PR addresses?

This PR resolves an issue where the publishConfig.provenance field in package.json wasn't being read correctly.

Specifically:

  • When publishConfig.provenance: true was set in package.json, Yarn's npm publish command wouldn't generate provenance statements
  • Whilst the PublishConfig interface had provenance?: boolean defined, the load method in Manifest.ts was missing the processing logic for the provenance field when handling publishConfig
  • Other fields (access, main, module, browser, registry, bin, executableFiles) were being processed correctly, but provenance was the only one being omitted

How did you fix it?

I added the following code to the publishConfig processing section within the load method in packages/yarnpkg-core/sources/Manifest.ts:

if (typeof data.publishConfig.provenance === `boolean`)
  this.publishConfig.provenance = data.publishConfig.provenance;

This fix ensures:

  1. The publishConfig.provenance field in package.json is read correctly when it's a boolean value
  2. It becomes available as workspace.manifest.publishConfig.provenance
  3. The existing logic in npm publish command (publish.ts lines 109-120) functions properly

Additionally, to ensure the accuracy of the fix, I added test cases to packages/yarnpkg-core/tests/Manifest.test.ts:

  • Test for when provenance: true is set
  • Test for when provenance: false is set
  • Test to verify that non-boolean values are ignored

With this fix, when publishConfig.provenance is configured in package.json, provenance statements will be generated as expected.

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.

@smorimoto smorimoto marked this pull request as ready for review May 2, 2025 13:16
@smorimoto smorimoto force-pushed the fix-value-check-publishconfig-provenance branch from d79f2ff to a138eff Compare May 2, 2025 13:20
@smorimoto
Copy link
Contributor Author

The failure of CI has nothing to do with this PR, and #6776 needs to be merged first.

Copy link
Member

@clemyan clemyan left a comment

Choose a reason for hiding this comment

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

It is not possible for workspace.manifest.publishConfig to have a provenance key but set to undefined, because workspace.manifest is parsed from JSON. So using in here is correct. In fact, this change causes { "publishConfig": { "provenance": false } } to be ignored.

The fact that publishConfig.provenance in manifests do not take effect is not a bug with the publish code, but rather a bug with the manifest parser in Manifest.ts in the core.

@smorimoto smorimoto force-pushed the fix-value-check-publishconfig-provenance branch from a138eff to ddfbfc1 Compare May 26, 2025 12:17
@smorimoto smorimoto changed the title fix(plugin-npm): fix the value check for publishConfig.provenance fix(core): fix the value check for publishConfig.provenance May 26, 2025
@smorimoto
Copy link
Contributor Author

@clemyan Correct! I just updated the PR!

Signed-off-by: Sora Morimoto <sora@morimoto.io>
@smorimoto smorimoto force-pushed the fix-value-check-publishconfig-provenance branch from ddfbfc1 to 9ba6b4e Compare May 26, 2025 12:25
@smorimoto smorimoto requested a review from clemyan May 26, 2025 12:25
@smorimoto
Copy link
Contributor Author

When do you expect this PR to be merged and released?

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