[code-infra] Modify publish script to filter packages#1236
[code-infra] Modify publish script to filter packages#1236brijeshb42 merged 2 commits intomasterfrom
Conversation
|
Alternatively, what if we support the |
Bundle size report
Check out the code infra dashboard for more information about this PR. |
929d3e2 to
c0a1edf
Compare
| type: 'string', | ||
| array: true, | ||
| description: | ||
| 'pnpm --filter expressions to select which packages to publish (same syntax as `pnpm -F`). When omitted, all public packages are published.', |
There was a problem hiding this comment.
Is that a union or intersection of the filter? How does it work in pnpm?
There was a problem hiding this comment.
Separate filters mean packages that satisfy any of the patterns. So union.
c0a1edf to
32dff0b
Compare
| const publishNames = new Set(packagesToPublish.map((pkg) => pkg.name)); | ||
| const depsOnly = await getWorkspacePackages({ | ||
| publicOnly: true, | ||
| filter: filter.map((f) => `${f}^...`), |
There was a problem hiding this comment.
I was thinking, we should probably widen this check. e.g. packages should not depend on workspace: packages that are not in the list returned from getWorkspacePackages, also when no filter is specified. e.g. what if it has a workspace dependency on a private package. I also don't think this trick would work correctly if the filter is something like foo-*.
I think we could probable generalize and reuse getTransitiveDependencies on all the packages, then check if it returned any package not in the list.
There was a problem hiding this comment.
Since the check is reused in multiple places, perhaps we should create a function for it as well:
function getExtraneousWorkspaceDependency(packageList: string[]): string[]or something, which returns a list of workspace packages that anything in packageList transitively depends on without including it.
a4aa4c0 to
64fbd90
Compare
|
|
||
| /** | ||
| * @typedef {Object} GetTransitiveDependenciesOptions | ||
| * @property {boolean} [includeDev=true] - Whether to include devDependencies in the traversal |
There was a problem hiding this comment.
should we also add includeProd then?
| * roots depend on it. | ||
| * | ||
| * @param {string[]} packageNames - Package names to start the traversal from | ||
| * @param {Map<string, string>} workspacePathByName - Map of workspace package name to directory path |
There was a problem hiding this comment.
I could go either way.
| * @param {GetTransitiveDependenciesOptions} [options] | ||
| * @returns {Promise<Set<string>>} All reachable workspace package names, including the input packages themselves | ||
| */ | ||
| export async function getTransitiveDependencies(packageNames, workspacePathByName, options = {}) { |
There was a problem hiding this comment.
workspacePathByName feels very leaky. One thing we could do is create a function that fetches this, but caches the results. Maybe something for follow-up
| * Sets of package names that violate the requirements. Both sets are empty when valid. | ||
| */ | ||
| export async function validatePublishDependencies(packages) { | ||
| const allWorkspacePackages = await getWorkspacePackages(); |
There was a problem hiding this comment.
Calling getWorkspacePackages again while it was already called to obtain packages is a bit wasteful. I'm not sure how we can fix this though without creating leaky abstractions. Perhaps some of the commands need to cache their results? We can do it in follow-up
There was a problem hiding this comment.
Yeah. This here is being done to get all public and private packages so we can track if something is not included in the publish list.
I think its fine though since the script code-infra publish or code-infra publish-canary is only called once in a release trigger. Can be looked into it separately.
64fbd90 to
70ed950
Compare
Janpot
left a comment
There was a problem hiding this comment.
Approving, but from a maintainability POV, would prefer to keep validation logic internal to validatePublishDependencies
This also includes a refactoring of existing transitive dependency checker in cmdNetlifyIgnore. It handles checking if the filtered publishable packages have any dependency that is not published or is private.
70ed950 to
0bf94b3
Compare
being published. The flag is same as
pnpm -Fand does the same thinginternally to call pnpm with the flag to get the list of packages to
publish.
This change is done to allow use to do two phase publishing in core
where we want one workflow job to publish only internal packages and the
other job to exclude such packages.
Refactored the code related to transitive dependency resolution and moved
it to pnpm utils module.
Tested locally with dry-run.
Related to - mui/material-ui#47952