-
-
Notifications
You must be signed in to change notification settings - Fork 941
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
feat: implement catalog protocol reads (feature flagged) #8020
Conversation
c3c6b50
to
f7d198f
Compare
@@ -0,0 +1,3 @@ | |||
# @pnpm/catalogs.types | |||
|
|||
> Types related to the pnpm catalogs feature. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of creating a new @pnpm/catalogs.types
package, we could also stuff the small definitions here into the existing @pnpm/types
package. I opted for a new package for now since we're doing something similar for hooks: @pnpm/hooks.types
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I'll separate the new packages out to a different PR so you don't have to keep looking at these changes when re-reviewing this PR.
@@ -0,0 +1,3 @@ | |||
# @pnpm/catalogs.protocol-parser |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the moment, this small package gets used in:
pkg-manager/core/src/install/index.ts
pkg-manager/resolve-dependencies/src/getCatalogResolver.ts
Thought it was worth breaking it out into a new package. The publishing packages will likely need this utility too in a future PR.
/** | ||
* The concrete version that the requested specifier resolved to. Ex: 1.2.3 | ||
*/ | ||
readonly version: string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the pnpm-lock.yaml
file, this will look like:
catalogs:
default:
foo:
specifier: ^1.2.3
version: 1.2.3
This PR writes the version
part. A future PR will read from it to optimize new usages of the catalog protocol. I think the logic will be similar to the existing replaceVersionInPref
logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if there will be multiple named catalogs with the same specs? will the entries be duplicated? Like
catalogs:
default:
foo:
specifier: ^1.2.3
version: 1.2.3
named:
foo:
specifier: ^1.2.3
version: 1.2.3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, they'll be duplicated. I think that's okay and somewhat by design if someone duplicates a catalog entry across multiple named catalogs.
}) | ||
}) | ||
|
||
test('lockfile catalog snapshots should keep unused entries', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Intentionally keeping around unused entries for now. Otherwise we'd wipe out catalog:
dependencies in a filtered install. I think we'll want to change this logic in another PR to prevent stale references though.
@@ -0,0 +1,3 @@ | |||
# @pnpm/catalogs.types | |||
|
|||
> Types related to the pnpm catalogs feature. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good to me
/** | ||
* The concrete version that the requested specifier resolved to. Ex: 1.2.3 | ||
*/ | ||
readonly version: string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if there will be multiple named catalogs with the same specs? will the entries be duplicated? Like
catalogs:
default:
foo:
specifier: ^1.2.3
version: 1.2.3
named:
foo:
specifier: ^1.2.3
version: 1.2.3
|
||
useBetaCatalogsFeat?: boolean |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a need for a setting? It is opt-in anyway, as the new field in pnpm-workspace.yaml
should be added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's mostly to avoid bug reports that catalogs aren't working on the next version (ex: 9.1.0). I was thinking it'd be better for it to not work entirely rather than partially work.
Without the setting here, the catalog:
protocol would work when defined pnpm-workspace.yaml
, but we wouldn't replace it on publish. I'm working on the publish replacing part next.
Alternatively we could:
- Use an environment variable to feature flag this.
- Avoid merging any catalog PRs until it's completely finished.
I thought the temporaryuseBetaCatalogsFeat
argument was the best out of those 3 options, but open to other thoughts!
@@ -163,7 +175,26 @@ export async function getContext ( | |||
extraBinPaths.unshift(path.join(hoistedModulesDir, '.bin')) | |||
} | |||
const hoistPattern = importersContext.currentHoistPattern ?? opts.hoistPattern | |||
|
|||
const [catalogs, lockfiles] = await Promise.all([ | |||
readCatalogsFromWorkspaceManifest(opts.lockfileDir, opts.useBetaCatalogsFeat ?? false), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the workspace manifest is already read outside of @pnpm/core
. The catalogs should be passed to core through options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's a better idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spent some time thinking through the best way to do this. It's a bit complicated, which is why I avoided it in this PR. But I do think it's a good idea to de-duplicate pnpm-workspace.yaml
reads. I was planning on doing it after this PR, but it's probably better to do it beforehand.
Here's our only usage of readWorkspaceManifest
in pnpm:
pnpm/workspace/find-packages/src/index.ts
Lines 39 to 44 in 9719a42
export async function findWorkspacePackagesNoCheck (workspaceRoot: string, opts?: { patterns?: string[] }): Promise<Project[]> { | |
let patterns = opts?.patterns | |
if (patterns == null) { | |
const workspaceManifest = await readWorkspaceManifest(workspaceRoot) | |
patterns = workspaceManifest?.packages | |
} |
A few approaches to refactor that:
- We could read
WorkspaceManifest
higher up and pass it tofindWorkspacePackages
. This would involve refactoring all the installation commands:add
,install
,import
, etc that are currently callingfindWorkspacePackages
in each of its handlers. - We could create a new
readWorkspaceManifestCached
option thatfindWorkspacePackages
uses.
I prefer option 1 since 2 requires a longer lived module-level cache, but option 1 involves much more refactoring.
@zkochan I can do approach 1 in a separate PR if that sounds good?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
approach 1 is the right one. Maybe we can read it in @pnpm/config
. We already read the root manifest there too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Next part here starting this here:
specifier: 'catalog:', | ||
version: '1.0.0', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if the version is also present in the catalogs object, then why is it duplicated here? I guess it makes sense to duplicated it in case when is-positive has peer dependencies. Probably should have such test when the same catalog is used in two projects and has peers that resolve to different versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, it's the peer dependencies part of this. Will do — I'll add a test for that.
export function getCatalogResolver (catalogs: Catalogs): CatalogResolver { | ||
return function resolveFromCatalog (wantedDependency: WantedDependency): CatalogResolution | undefined { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to use a nested function. Just use bind.
export function getCatalogResolver (catalogs: Catalogs): CatalogResolver { | |
return function resolveFromCatalog (wantedDependency: WantedDependency): CatalogResolution | undefined { | |
export function resolveFromCatalog (catalogs: Catalogs, wantedDependency: WantedDependency): CatalogResolution | undefined { |
Thanks for the review. I'll move to draft while I work on the feedback. |
In your PR you mention several issues with filtered install. I think we need to change how filtering works as we have many issues with it now. If the lockfile is not up-to-date, when a filtered install happens, we should do a non-filtered full resolution only install first. This will be less terrible in pnpm v9 as in v9 pnpm doesn't download the package tarballs during resolution. |
I like this idea. It should also help with the folks confused by how I'm going to reopen this PR without the feature flag since we're merging into a feature branch. |
Changes
This implements the following items from the tracking issue #7072.
catalog:
andcatalog:default
specifiers.catalog:<name>
specifiers.I've organized this PR into different commits that make sense by themselves. This PR should be easier to review commit by commit rather than all files changed at once.
When should we transform the catalog protocol?
The hardest part of this PR was determining the right place for the
catalog:
→^1.2.3
translation internally.pnpm.overrides
andpackageExtensions
work. I think this is too early of a place for the protocol to be resolved since it means thepnpm-lock.yaml
file won't have the originalcatalog:
specifier that was declared inpackage.json
.@pnpm/default-resolver
level, but I think that's too deep. It means the default resolver would need to understand catalogs, which leaks the catalogs abstraction and makes the resolvers more complicated.I settled on performing the
catalog:
protocol replacement right before we request dependencies from the store. I think that strikes the right balance.pnpm/pkg-manager/resolve-dependencies/src/resolveDependencies.ts
Lines 1135 to 1141 in ade62a9