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

Add support for specifying --access, aligning better with default publish behavior #81

Merged
merged 2 commits into from
Oct 17, 2024

Conversation

saracope
Copy link
Contributor

@saracope saracope commented Oct 17, 2024

Closes #80

Changes:

  • Update the --dry-run flag so it is passed to pnpm publish to get more useful feedback.
  • Remove the hardcoded --access=public argument and instead allow users to pass the --access flag with public or restricted set.

Previously, running pnpm release-plan publish would force any package to publish as public, even if the access was set to restricted in the package.json.

This change better matches the default behavior of pnpm publish and allows scoped packages to avoid being switched to public accidentally.

With this change, the expected behaviors are:

  1. Running publish without an --access flag will respect the publishConfig access set in package.json
  2. Running publish with an --access flag will override the publishConfig access set in package.json
  3. Running publish without an --access flag and no publishConfig access set in package.json
    1. if your package is scoped, it will publish with restricted access
      "By default, scoped packages are published with private visibility." [source]
    2. otherwise, it will use the default publish
      "By default npm will publish to the public registry. This can be overridden by specifying a different default registry or using a scope in the name, combined with a scope-configured registry (see package.json)." [source]

Important

Item 3 here could use extra consideration because I think it could cause a breaking change for folks who do not set the access in their package.json or publish command.


@wycats
Copy link

wycats commented Oct 17, 2024

I think it makes sense for release-plan to have the same behavior as normal npm/pnpm here. That means that scoped packages that publish using release-plan would use publishConfig.access to specify that they should be published using --access publish.

This has a few upsides:

  1. This communicates the package's access to other tools that might rely on default npm behavior.
  2. By relying on the default behavior of pnpm, we also pick up other ways that you might have configured the access setting.
  3. It is also forward-compatible with potential future changes in either npm or pnpm that further adjust how the access is configured.

The TL;DR is that this PR removes the hardcoded --access public entirely, which buys us compatibility with the standard ways of configuring the access.


However, this would be a breaking change, because many existing release-plan packages do not currently configure their access.

I personally think that release-plan apps should explicitly configure publishConfig.access, which would make their public access visible to other tools.

But the vast majority of existing release-plan packages are Open Source packages without an explicitly configured access, and this would break their builds. How should we deal with the breaking change? A couple of options:

  1. Cut an 0.10 release, document the breaking change and migration (i.e. set "publishConfig.access": "public" in package.json or in .npmrc).
  2. Attempt to determine whether the user has configured access (by reading package.json and calling pnpm config get access). If the didn't configure access, stick to the current default behavior, and instruct the user to update their package.json.

@NullVoxPopuli
Copy link
Contributor

NullVoxPopuli commented Oct 17, 2024

However, this would be a breaking change, because many existing release-plan packages do not currently configure their access.

as it turns out, it's only a breaking change for packages that haven't yet published any version at all, because once you publish a public scoped package, npm knows that it's public, and keeps going with that.

Relying on default behavior protects us nicely here

"--tag=best-tag",
],
"released": Map {},
}
`);
});

it('adds dry-run if passed by options', async function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

🙌

@NullVoxPopuli NullVoxPopuli added the enhancement New feature or request label Oct 17, 2024
Copy link
Contributor

@NullVoxPopuli NullVoxPopuli left a comment

Choose a reason for hiding this comment

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

thank you!!!

@NullVoxPopuli NullVoxPopuli changed the title Add --access flag Add support for specifying --access, aligning better with default publish behavior Oct 17, 2024
@NullVoxPopuli NullVoxPopuli merged commit 190c636 into embroider-build:main Oct 17, 2024
4 checks passed
@github-actions github-actions bot mentioned this pull request Oct 17, 2024
@mansona
Copy link
Member

mansona commented Oct 21, 2024

Can I just say, this is a great PR 😍

It's also fantastic to take a weekend off and come back to a whole loop being closed! Great work everyone! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Publish private package
4 participants