Skip to content

feat: read catalog configs from workspace manifest (feature flagged)#8019

Merged
zkochan merged 4 commits intopnpm:mainfrom
gluxon:pnpm-workspace-catalogs-config
Apr 26, 2024
Merged

feat: read catalog configs from workspace manifest (feature flagged)#8019
zkochan merged 4 commits intopnpm:mainfrom
gluxon:pnpm-workspace-catalogs-config

Conversation

@gluxon
Copy link
Member

@gluxon gluxon commented Apr 26, 2024

Changes

Part of #7072.

This PR reads pnpm catalogs in WorkspaceManifest and adds type definitions for them. Future pull requests will read from this config.

The changes are feature flagged for now to prevent the catalogs feature from being accidentally used before it's fully implemented.

Next

The next PR is #8020, but I'll leave that in draft mode until this PR merges.

Comment on lines +47 to +52
// Disable catalogs config reads by default until the overall feature is ready.
const isCatalogsConfigEnabled = opts?.catalogs ?? false
if (!isCatalogsConfigEnabled && typeof manifest === 'object' && manifest != null) {
delete (manifest as { catalog?: unknown }).catalog
delete (manifest as { catalogs?: unknown }).catalogs
}
Copy link
Member Author

Choose a reason for hiding this comment

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

The intention of feature flagging this was to prevent validation errors if users attempted to define catalogs today. I think a user would reasonably assume catalogs were ready as an overall feature if pnpm validated its config, but that's not true today.

}

for (const [alias, specifier] of Object.entries(manifest.catalog)) {
if (typeof specifier !== 'string') {
Copy link
Member Author

@gluxon gluxon Apr 26, 2024

Choose a reason for hiding this comment

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

I briefly considered supporting integers. For example:

catalog:
  react: 18

While I'd personally use ^18.3.0, 18 is technically a valid specifier. Currently the above would throw and users would have to write:

catalog:
  react: "18"

I think we should support plain integers as strings. Does that seem reasonable? I can do that in a following PR if so.

Copy link
Member

Choose a reason for hiding this comment

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

no, let's only support it as string. Otherwise it becomes confusing as 18.10 isn't bigger than 18.2 according to semver.

Copy link
Member Author

Choose a reason for hiding this comment

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

I definitely wouldn't support floats for the reason you mentioned. But yeah, even supporting just integers becomes a slippery slope. Okay, let's just do strings. 🙂

@gluxon gluxon changed the title feat: read catalogs config from workspace manifest (feature flagged) feat: read catalog configs from workspace manifest (feature flagged) Apr 26, 2024
@gluxon gluxon marked this pull request as ready for review April 26, 2024 08:00
@gluxon gluxon requested a review from zkochan as a code owner April 26, 2024 08:00
@zkochan zkochan merged commit 773434e into pnpm:main Apr 26, 2024
@gluxon gluxon deleted the pnpm-workspace-catalogs-config branch April 26, 2024 21:22
zkochan added a commit that referenced this pull request Apr 29, 2024
zkochan pushed a commit that referenced this pull request Apr 29, 2024
…8019)

* refactor: move InvalidWorkspaceManifestError to its own file

* feat: read catalogs config from workspace manifest

* feat: disable catalogs config reads by default

* chore: add changeset for new catalog config parsing
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