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

RFC: Catalogs β€” Shareable dependency version specifiers #1

Merged
merged 14 commits into from
Sep 8, 2023

Conversation

gluxon
Copy link
Member

@gluxon gluxon commented Mar 27, 2022

This is an RFC for a new catalog: version specifier protocol in pnpm. All comments and feedback welcome. πŸ™‚

Edit July 7th, 2023: A previous version of this RFC was titled "RFC: First-Class Support for Workspace Consistent Versions". We've renamed the proposal to "Catalogs" following a similar idea from the Gradle build tool.

Previous discussions:

Comment on lines 457 to 459
"@types/react": "^17.0.43",
"react": "^17.0.2",
"react-dom": "^17.0.2",

Choose a reason for hiding this comment

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

For publishing, I think it would be most useful if the definition of the version group defined both the exact version that is to be installed in the workspace, and separately the specifier that is to be written during publishing. This will allow developers to loosen the version for their customers while still controlling exactly what gets installed in the workspace (which is increasingly necessary in the current security landscape).

Copy link
Member Author

Choose a reason for hiding this comment

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

Open to this, but want to ask a few more questions.

  1. Does specifying both a range specifier for internal upgrades and publishing run into the scenario described here? Namely:

    • If you have an internal upgrade range of ^17.0.0, and a resolved internal lockfile version of 17.0.1 what should a publish range of * result in? 17.0.0 or 17.0.1?
    • In a similar scenario but a publish range of ^, should the dependency publish with ^17.0.0 or ^17.0.1? I think the answer to this scenario is more clearly ^17.0.0, but it results in confusing behavior viewed next to the ambiguous behavior for *.
  2. I might be missing something, but I think this is the existing situation for how dependencies are declared and resolved at the moment too? Is there something special about workspace consistent versions that means this feature should do something additional?

    • To elaborate, if someone wants to keep the version range for customers loose while controlling exactly what gets installed into the workspace, they have to modify the pnpm-lock.yaml file (either manually or through a command).

Choose a reason for hiding this comment

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

I'm a bit confused by your linked example, in that I'd expect the only data in package.json to be "react": "workspace-consistent:react-rc". The version range is specified in the definition of "react-rc". I'd like to be able to do something like:

"react-rc": {
  "react": {
    "publish": "^17",
    "install": "17.0.2"
  }
}
"dependencies": {
  // Resolves to "17.0.2" at install time, when published is "^17"
  "react": "workspace-consistent:react-rc"
}

The value in "publish" is a superset of the value in "install". Technically you could do the same thing by declaring two dependency sets "react-rc-dev" and "react-rc-publish" and having the former as devDependencies and the latter as regular dependencies, since devDependencies supersede regular dependencies in local packages, but that's rather cumbersome compared to declaring it inside the definition of the version group.

Copy link
Member Author

@gluxon gluxon Jun 23, 2022

Choose a reason for hiding this comment

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

Apologies for the late response. I do appreciate your input.

Are there downsides to keeping the resolved version in pnpm-lock.yaml? Adapting your example:

"react-rc": {
  "react": "^17"
}
# pnpm-lock.yaml

workspaceConsistent:
  react-rc:
    react:
      specifier: ^17
      dependency: 17.0.2

Although it may not be intuitive, it's possible to edit the dependency: 17.0.2 line in pnpm-lock.yaml instead of editing an "install": "17.0.2" field in package.json. I think this accomplishes the same thing?

The benefits of keeping resolved versions in pnpm-lock.yaml are:

  • Follows the current convention of keeping concrete/resolved versions in pnpm-lock.yaml.
  • pnpm update doesn't have to edit the root package.json, which I think can become strange. Admittedly I haven't tested this command before, but from the description in the docs I'd imagine it only edits pnpm-lock.yaml at the moment.
  • Users don't have to manually look up or decide concrete versions on npmjs.com when adding a new dependency.

I do see the benefit of having resolved versions in package.json being:

  • It's apparent to users that you can edit the resolved version in package.json. (Editing pnpm-lock.yaml is probably discouraged on most teams.)

Is everything above correct? Did I misunderstand the pros/cons?

Without the colon, this conflicts with the existing npm dist tag
namespace.

pnpm#1 (comment)
The original @types/react example wasn't great.

pnpm#1 (comment)

As monorepos grow in workspace package count, merge conflicts become increasingly more probable. On GitHub, any merge conflict in `pnpm-lock.yaml` blocks pull requests due to lack of custom merge driver support.

With first-class support for workspace consistent versions, edits on upgrades are limited to the root `package.json` and the `workspaceConsistent` portion of `pnpm-lock.yaml`. This reduces churn in `pnpm-lock.yaml`, and **prevents merge conflicts in all the cases listed above**.
Copy link
Member

Choose a reason for hiding this comment

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

I don't get it, how does it reduce the nubmer of conflicts? The same conflicts will happen just in another place, in the workspaceConsistent section.

Copy link
Member Author

@gluxon gluxon Mar 30, 2022

Choose a reason for hiding this comment

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

The spacing in the workspaceConsistent section helps a lot. I took some time tonight to commit the example merges in this RFC to a GitHub repo. I added a theoretical pull request for each scenario in an imaginary world using the workspace-consistent: protocol.

For the PRs in this repo:

  • The main branch has just upgraded React.
  • The main-workspace-consistent branch has also just upgraded React, but is using the workspace-consistent: protocol.

Note that all PRs using workspace-consistent: show as ready to merge in GitHub, while the commits not using the protocol have merge conflicts in pnpm-lock.yaml.

Example Not Using the New Protocol Using the New Protocol
Changes the version of a dependency line-adjacent to the upgraded dependency.
Add or removes a dependency line-adjacent to the upgraded dependency.
Adds a new declaration of the upgraded dependency.

Copy link
Member

Choose a reason for hiding this comment

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

Looks like simply adding new lines also solves the issue: gluxon/pnpm-workspace-consistent-examples#7

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for testing that alternative and adding an example. Mentioned in a related thread #1 (comment), but that does solve 1 and 2.

I am still interested in this RFC since:

  • It addresses the 3rd "Adds a new declaration of the upgraded dependency." case that the spacing doesn't.
  • Allows package.json files to be more merge conflict resistant too without adding newlines to the dependencies section. I could see slow adoption for adding newlines to dependencies as a best practice in repos with consistent versions. Whereas if repos use this feature, they get the merge conflict resistance for free.
  • Having workspace-consistent: in package.json still better reflects intent.
    • Namely, the "How do I add a new dependency?" is common question amongst new developers on my team. The current answer is you go check if another package.json has it listed and copy the version range if it does exists. That feels a bit strange. Having pnpm add <pkg> simply added "<pkg>": "workspace-consistent:" if <pkg> is configured feels much better.

react-dom: 17.0.2
```

The alternative format better matches the existing shape of `importers`. However the proposed format is more resilient to merge conflicts. Separate commits changing the version of `react` and `jest` would merge conflict with the alternative format, while the proposed format would merge those changes cleanly.
Copy link
Member

Choose a reason for hiding this comment

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

OK, so then wouldn't it be enough to change the format of the importers? Merge conflicts would be fixed without adding the complex workspaceConstraints logic to the lockfile.

Copy link
Member Author

Choose a reason for hiding this comment

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

Looking at the scenarios mentioned:

  1. Changes the version of a dependency line-adjacent to the upgraded dependency.
  2. Add or removes a dependency line-adjacent to the upgraded dependency.
  3. Adds a new declaration of the upgraded dependency.

You're right. I think a new format for importers would address 1 and 2. 3 isn't handled, but that may be good enough. Will think about this a bit more.

Choose a reason for hiding this comment

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

I think the merge conflicts in pnpm-lock.yaml are less of an issue (the current format helps a lot) than merge conflicts in package.json when upgrading dependencies across the repository. A common situation that I run into is one developer adding a new project that uses the common version of a dependency colliding with a PR that upgrades said dependency. This actually manifests not as a git merge conflict but as a break in main due to an invalid lockfile and/or breaking the checker (since my team uses Rush we have version consistency checking during install, which fails in this case).

Copy link
Member

Choose a reason for hiding this comment

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

@gluxon I think we can implement this for importers and solve a big chunk of the issues. It is not hard to implement, so we can do it as a breaking change to the lockfile format in pnpm v7.

I think workspace constraints are a bigger change. I don't know yet if it is a good idea to put so much additional logic to the lockfile. In any case, this will require a lot of time for review.

Maybe there could be alternative solutions to solve the issues where the version is influenced by peer deps.

Like instead of this:

importers:
  project:
    dependencies:

      react:
        specifier: ^1.0.0
        version: [email protected]

      react-dom:
        specifier: ^1.0.0
        version: 1.0.0

we could do this

importers:

  project:
    dependencies:

      react:
        specifier: ^1.0.0
        version: ['1.0.0', 'react-dom']

      react-dom:
        specifier: ^1.0.0
        version: 1.0.0

Copy link
Member Author

@gluxon gluxon Mar 31, 2022

Choose a reason for hiding this comment

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

The situation @dmichon-msft described is something I see happen about once a week on my team. It's a flavor of the "3. Adds a new declaration of the upgraded dependency." conflict type (mentioned above) that the changes to the importers section described in the above comment would not address. At that frequency, it's not the most pressing problem. I tend to get an alert that our CI is broken, run pnpm install, and force push any updated package.json and pnpm-lock.yaml files to develop.

Tangent on bors

Last year I was trying to convince my team to adopt bors as another way to prevent any kind of develop branch breaks, but I think we'll just switch to the GitHub merge queue when it's available on GitHub Enterprise instead of asking everyone to learn how bors works only to switch away from it later.

I think we can implement this for importers and solve a big chunk of the issues.

I think you're right. This solves 1 and 2 (#1 (comment)), which is indeed a big chunk of the problem.

Another tangent on pnpm-lock.yaml and newlines:

In January I ended up writing a space-lockfile command that is set to run on postinstall. It simply goes through pnpm-lock.yaml and adds any newlines before/after lines that contain an element in a list of package names. (The package names in this list are the consistent versions in the repo.) This made pnpm-lock.yaml merge conflict much less and has worked well since it was introduced 2 months ago, but the readability of the file was sacrificed a bit. I'm not completely satisfied with my space-lockfile solution either. If pnpm had a hook for serializing/deserializing lockfiles, I would have implemented a minimal version of this RFC without version groups with it instead.

It is not hard to implement, so we can do it as a breaking change to the lockfile format in pnpm v7.

I appreciate you being open to this. I am a bit worried making the importers sections 25% newlines might cause other unexpected problems though, and wouldn't want to risk pnpm v7's release since it seems like we're in the beta phase already. Is it possible for these formatting changes to have edge cases where commits that merged cleanly before no longer do with the new format? Even if that is possible, perhaps that's a non-concern if it decreases the probability of conflicts as a whole.

I think workspace constraints are a bigger change. I don't know yet if it is a good idea to put so much additional logic to the lockfile. In any case, this will require a lot of time for review.

I was imagining the workspace-consistent: protocol changes to primarily happen during pnpm-lock.yaml serialization and deserialization. Everything else in pnpm would see the same PnpmLockfile type as it does now. (With a workspaceConsistent key that can be ignored.) Does that reduce the scope/risk of this change? Or is it strange to implement in this manner? There might need to be some tweaking to the peersSuffix format when there's a hash involved for this to happen during (de)serialization, but it feels doable.

Maybe there could be alternative solutions to solve the issues where the version is influenced by peer deps.

I like the direction this idea is heading. I think the peersSuffix portion (especially when it's a hash) is the only non-intuitive part of the lockfile. I was only able to understand the concept because you explained it well here.

Copy link
Member Author

@gluxon gluxon Apr 27, 2022

Choose a reason for hiding this comment

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

I think we can implement this for importers and solve a big chunk of the issues. It is not hard to implement, so we can do it as a breaking change to the lockfile format in pnpm v7.

@zkochan I'd actually like to experiment with this a bit. We proposed this format above:

importers:

  project:
    dependencies:

      react:
        specifier: ^1.0.0
        version: [email protected]

      react-dom:
        specifier: ^1.0.0
        version: 1.0.0

But I think we can just do:

importers:

  project:
    dependencies:
      react:
        specifier: ^1.0.0
        version: [email protected]
      react-dom:
        specifier: ^1.0.0
        version: 1.0.0

I actually don't think newlines are strictly required since react: and react-dom: are on their own lines and not changing as much as specifier or version. Git can use those lines to determine where diff regions apply rather than the newlines.

@zkochan When I start experimenting, would it be preferable to localize this change to the lockfile serialization/deserialization, or update the PnpmLockfile shape itself? I lean towards the later despite the wider surface area it'll affect.

Copy link
Member

Choose a reason for hiding this comment

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

For now, I would localize the change to serialization/deserialization. Because we will probably hide the new format under a feature flag. Of course, when we'll be sure about the format, we will make it the default and refactor PnpmLockfile.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for discussing! Filing an issue on myself to track: pnpm/pnpm#4725

@gluxon
Copy link
Member Author

gluxon commented Jan 4, 2023

@zkochan I know you still had reservations back when we discussed this last year (understandably). Have the feelings leaned one way or the other since then? Despite the new (currently feature flagged) lockfile format, I'm still very interested in this for the reasons in #1 (comment). I think this would be a very popular pnpm feature if it lands.

Unless you were heavily opposed, I was going to start a prototype pull request for resolving workspace-consistent: specs, and follow that up with the lockfile changes. I think that would be a helpful gauge of implementation complexity since that was the main concern.

@zkochan
Copy link
Member

zkochan commented Jan 8, 2023

See my suggestions for the new lockfile format in pnpm/pnpm#5810

I am not against a workspace-consistent feature but adding it to the lockfile I don't believe is a good idea.

  • it won't solve all the cases because it will only use workspace-consistent: when the peer dependency is resolved from a direct dependency.
  • it adds complexity to the lockfile parsing logic, which is already incredibly complex. Especially, now that we try to change its format.

Maybe with the new format, we could do a simpler syntax in the lockfile. Something like this:

  /@typescript-eslint/[email protected](eslint)(typescript):
    resolution: {integrity: sha512-x2jrMcPaMfsHRRIkL+x96++xdzvrdBCnYRd5QiW5Wgo1OB4kDYPbC1XjWP/TNqlfK93K/lUL92erq5zPLgFScQ==}
    engines: {node: ^12.22.0 || ^14.17.0 || >=16.0.0}
    peerDependencies:
      eslint: '*'
    dependencies:
      '@types/json-schema': 7.0.11
      '@types/semver': 7.3.13
      '@typescript-eslint/scope-manager': 5.48.0
      '@typescript-eslint/types': 5.48.0
      '@typescript-eslint/typescript-estree': 5.48.0(typescript)
      eslint: 8.31.0
      eslint-scope: 5.1.1
      eslint-utils: 3.0.0(eslint)
      semver: 7.3.8
    transitivePeerDependencies:
      - supports-color
      - typescript
    dev: false

@gluxon
Copy link
Member Author

gluxon commented Jan 20, 2023

Thanks! I can get a PR together that uses workspace-consistent: in the specifiers portion of pnpm-lock.yaml importers. We can keep the existing peersSuffix and packages dep path portions the same. That should simplify this a lot. It doesn't enable all the merge conflict resistance benefits I was aiming for, but gets much closer than the current state today. Does that sound like a good plan?

Future looking, do you think it's possible to make improvements to the lockfile parsing logic that simplify it? I'm curious if you think the logic itself is complex, or if it's the existing implementation. I'm happy to help work on the lockfile parsing parts of pnpm for a bit if we think implementation can be simplified.

@zkochan
Copy link
Member

zkochan commented Jan 21, 2023

workspace-consistent does not solve all the challenges that we have with sharing dependencies between projects. See the suggestion I have with environments: pnpm/pnpm#2713 (comment)

In theory, such environments could also be used to "compress" the lockfile.

But we can also think out of the box and redesign the lockfile completely to minimise potential conflicts.

@gluxon
Copy link
Member Author

gluxon commented Jan 22, 2023

workspace-consistent does not solve all the challenges that we have with sharing dependencies between projects. See the suggestion I have with environments: pnpm/pnpm#2713 (comment)

I actually think the environments idea that came up earlier this week is very similar to the proposal here. The concepts translate very closely:

  • An environment is similar to a version group. The @comp/react-env example given in the link above would be similar to defining a react-env version group in pnpm.workspaceConsistent.
  • The dependenciesFromEnv, devDependenciesFromEnv, and peerDependenciesFromEnv fields are similar to the workspace-consistent:<version-group> protocol. Both specify that a dependency should come from some other inherited definition.

Sharing and consuming environments

One important difference is that the environment (e.g. @comp/react-env) is a reusable package itself that can be published shared across different repositories. That can't be done with the root-level pnpm.workspaceConsistent definition. I think that's clever and like this property of environments.

I could certainly see this being used on my team, which has multiple monorepos we work on.

Defining consistent version usage

From the other thread, I saw that the configuration and properties of the environments proposal was still in discussion. pnpm/pnpm#2713 (comment)

I don't know how this will be configured and how the properties will be named.

If I may offer my opinion, I think a new protocol would offer more improvements than new dependenciesFromEnv, devDependenciesFromEnv, or peerDependenciesFromEnv fields.

Instead of:

{
  "name": "foo",
  "version": "1.0.0",
  "pnpm": {
     "environment": "@comp/[email protected]",
     "dependenciesFromEnv": ["react"],
     "devDependenciesFromEnv": ["eslint"],
     "peerDependenciesFromEnv": ["react-dom"]
  }
}

I think we should strongly consider:

{
  "name": "foo",
  "version": "1.0.0",
  "pnpm": {
     "environment": "@comp/[email protected]"
  },
  "dependencies": {
    "react": "pnpm-env:"
  },
  "devDependencies": {
    "eslint": "pnpm-env:"
  },
  "peerDependencies": {
    "react-dom": "pnpm-env:"
  }
}

(Edit: The first version of the block above kept the *[dD]ependenciesFromEnv fields by mistake. Thanks to @zkochan for catching.)

The reasons are:

  • Dependencies should be defined in dependencies. Developers then don't have to look at multiple parts of package.json to determine the entire set of dependencies, devDependencies, or peerDependenciespulled in. In a largepackage.json` file, I could see someone not knowing about the pnpm specific environments feature and being confused about how extra dependencies are getting added.
  • The package.json can't end up in an invalid state where the same dependency is added from both dependencies and dependenciesFromEnv. We'd have to catch when a package is declared in both of these blocks and throw an error otherwise. Reusing the existing dependencies block makes it much easier for developers editing package.json to catch this error. Many editors (e.g. VS Code) flag duplicate JSON keys out of the box.
  • Each of the *FromEnv blocks would have to be folded into the standard blocks anyway at publish time. For example, all the dependenciesFromEnv entries would have to be moved into dependencies on pnpm publish. Keeping these entries in the standard blocks better mirrors what package.json looks like before and after publishing. There's less publish time magic. I think users are already familiar with workspace: protocol versions that similarly get replaced at publish time anyway, so defining these in a protocol (e.g. pnpm-env: or workspace-consistent:) reuses existing concepts.

In theory, such environments could also be used to "compress" the lockfile.

How would dependencies from environments be represented in the lockfile? Would be interested in iterating with you on that if you're up for it.

But we can also think out of the box and redesign the lockfile completely to minimise potential conflicts.

Sweet. I'm glad to hear. ❀️

@zkochan
Copy link
Member

zkochan commented Jan 22, 2023

If I may offer my opinion, I think a new protocol would offer more improvements than new dependenciesFromEnv, devDependenciesFromEnv, or peerDependenciesFromEnv fields.

Sounds good, but in your example you have not removed the *FromEnv fields. Did you mean to suggest this?

{
  "name": "foo",
  "version": "1.0.0",
  "pnpm": {
    "environment": "@comp/[email protected]"
  },
  "dependencies": {
    "react": "pnpm-env:"
  },
  "devDependencies": {
    "eslint": "pnpm-env:"
  },
  "peerDependencies:" {
    "react-dom": "pnpm-env:"
  }
}

I think your points are valid. We can use this syntax instead (not sure how the protocol will be named yet).

The envs feature probably requires its own RFC and we can discuss there if we need changes in the lockfile for envs.

@zkochan
Copy link
Member

zkochan commented Jan 22, 2023

One more problem that I want to solve with envs is proxy packages that people use just to get their subdeps hoisted to the root of node_modules. But the issue with these is that they will want all deps from the env be installed. They don't want to list the deps.

I guess we could have an option to autoinstall all dev deps from the env as dev deps.

@gluxon
Copy link
Member Author

gluxon commented Jan 22, 2023

I think your points are valid. We can use this syntax instead (not sure how the protocol will be named yet).

Thanks for considering the points. Appreciate it.

For the name, I saw a suggestion for catalog: and liked it a lot. pnpm/pnpm#2713 (comment). (Or maybe version-catalog: so it's more easily searchable online.) The "environment" terminology might overlap with the existing pnpm env command. I think most people will have a good idea what "shareable version catalog package" is.

The envs feature probably requires its own RFC and we can discuss there if we need changes in the lockfile for envs.

Would you be open to me drafting the initial RFC this week? I think it's a very close evolution of the RFC here and want to help build it.

One more problem that I want to solve with envs is proxy packages that people use just to get their subdeps hoisted to the root of node_modules. But the issue with these is that they will want all deps from the env be installed. They don't want to list the deps.

I guess we could have an option to autoinstall all dev deps from the env as dev deps.

Interesting. Is the proxy package technique elaborated on somewhere? If someone needs those deps hoisted to the root node_modules, do "proxy packages" accomplish something that adding to the root package.json's devDependencies doesn't solve? Just want to make sure we fully solve the use case in the new RFC.

@zkochan
Copy link
Member

zkochan commented Jan 23, 2023

For the name, I saw a suggestion for catalog: and liked it a lot. pnpm/pnpm#2713 (comment). (Or maybe version-catalog: so it's more easily searchable online.) The "environment" terminology might overlap with the existing pnpm env command. I think most people will have a good idea what "shareable version catalog package" is.

I think we can use environments not only for versions. For instance, we could share scripts with it. We could share patch files to patch deps. We could also share the engines, license and author fields with it.

We can rename the pnpm env command to something else if that is a problem. For instance, to pnpm node-env

Would you be open to me drafting the initial RFC this week? I think it's a very close evolution of the RFC here and want to help build it.

ok

Interesting. Is the proxy package technique elaborated on somewhere? If someone needs those deps hoisted to the root node_modules, do "proxy packages" accomplish something that adding to the root package.json's devDependencies doesn't solve? Just want to make sure we fully solve the use case in the new RFC.

They don't want to add any dependencies to any package.json. They create a package with all tools that they need for developing an app in their company or team. So they would create a package that has esbuild, eslint, typescript, jest, etc in its dependencies. Then they install this single package to their dev dependencies and they expect esbuild, eslint, typescript and jest to be available in the root of node_modules. This doesn't work with pnpm and they consider it to be a bug in pnpm.

I guess we could have some field in the env's package.json to list dependencies that should always be installed as dev deps.

{
  "name": "@comp/utils",
  "version": "1.0.0",
  "alwaysIncludedDevDeps": ["jest", "typescript"],
  "dependencies": {
    "jest": "17",
    "typescript": "4",
    "ramda": "15"
  }
}

But I am not sure this feature is needed in the first iteration of the feature.

@gluxon
Copy link
Member Author

gluxon commented Jan 23, 2023

I think we can use environments not only for versions. For instance, we could share scripts with it. We could share patch files to patch deps. We could also share the engines, license and author fields with it.

Oh that's interesting. I do agree this is something most monorepos end up needing. I use a tool that checks whether a package.json has those fields with the expected license, repository, and license fields, but have seen other tools like https://github.com/microsoft/package-inherit in the past.

We can rename the pnpm env command to something else if that is a problem. For instance, to pnpm node-env

πŸ‘

They don't want to add any dependencies to any package.json. They create a package with all tools that they need for developing an app in their company or team. So they would create a package that has esbuild, eslint, typescript, jest, etc in its dependencies. Then they install this single package to their dev dependencies and they expect esbuild, eslint, typescript and jest to be available in the root of node_modules. This doesn't work with pnpm and they consider it to be a bug in pnpm.

Haha, well this "bug in pnpm" is my favorite feature. I could see why this company or team landed on the solution they did though.

I'm sure shamefully-hoist came up in your discussions with this team. What are the things this new env feature would accomplish shamefully-hoist doesn't?

I guess we could have some field in the env's package.json to list dependencies that should always be installed as dev deps.

{
  "name": "@comp/utils",
  "version": "1.0.0",
  "alwaysIncludedDevDeps": ["jest", "typescript"],
  "dependencies": {
    "jest": "17",
    "typescript": "4",
    "ramda": "15"
  }
}

But I am not sure this feature is needed in the first iteration of the feature.

Sounds good!

@gluxon
Copy link
Member Author

gluxon commented Jan 23, 2023

Sounds good, but in your example you have not removed the *FromEnv fields. Did you mean to suggest this?

Forgot to respond to this. Yes, that example should have removed the *FromEnv fields. Thanks for catching!

@zkochan
Copy link
Member

zkochan commented Jan 23, 2023

actually, we could also probably share overrides, packageExtensions and other settings from the pnpm field of package.json via environments.

maybe even some settings, like hoist-pattern and public-hoist-pattern.

@gluxon
Copy link
Member Author

gluxon commented Jan 23, 2023

actually, we could also probably share overrides, packageExtensions and other settings from the pnpm field of package.json via environments.

maybe even some settings, like hoist-pattern and public-hoist-pattern.

Sounds doable!

Since the discussion has evolved beyond the initial scope of this RFC, I'm hoping to ask more questions in a short-lived discussion specific to the environments feature. pnpm/pnpm#5974.

Would want to pick your brain a bit more in that discussion to make sure the new draft RFC matches your vision as closely as possible.

@gluxon
Copy link
Member Author

gluxon commented Jun 26, 2023

Going to close this in favor of #3 for now. The ideas translate fairly closely.

  • The workspace-consistent: specifier is essentially the newly proposed catalog: specifier.
  • Version groups are now different catalog templates. Each catalog template is a package.json file that can be local or externally consumed from the NPM registry.

A few ideas don't currently carry over to the new RFC, but I'm optimistic we can revisit them in some manner:

  • The enforceConsistencyTransitively option doesn't have an equivalent. I could imagine a separate feature similar to Yarn Constraints some day making it into pnpm that would accomplish the same thing.
  • Some of the merge conflict resistance features didn't make it, but that's mostly because we didn't reach a consensus in this RFC either.

I kept this PR open for a bit because there's an alternative design where the pnpm.workspaceConsistent config block proposed in this RFC merges, but a flavor of pnpm templates also merges that shares/distributes pnpm.workspaceConsistent. I think that's mostly a stylistic alternative at this point.

Thanks for everyone who reviewed this RFC! I'm looking forward to #3 accomplishing the same goals.

@gluxon gluxon closed this Jun 26, 2023
@gluxon
Copy link
Member Author

gluxon commented Aug 30, 2023

@pnpm/collaborators We'll need at least 1 more approving review to merge/ratify. Would another collaborator have time to read through the RFC and give thoughts? Thanks!

Copy link

@mcmxcdev mcmxcdev left a comment

Choose a reason for hiding this comment

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

Thanks for the detailed RFC and it's a really smart idea!

So catalog: would replace the functionality from what react": "workspace:* does currently across multiple packages? Or I guess it's more advanced since you can specify multiple different catalogs for packages depending on different versions of a dependency.

From what I have seen, https://github.com/JamieMason/syncpack which you also mentioned works quite well with keeping deps in sync, so I would consider this more "nice to have".

Are there any related GH issues in the pnpm repo that specifically wish for this functionality? Is there any real user interest in having this apart from some upvotes of this RFC?

Personally, since there are always limited resources in OSS, I would consider bug fixes and perf improvements to pnpm more important than this feature. But then again, I don't have to struggle with huge monorepos, so I don't feel the pain.

Comment on lines 112 to 130
// Can be referenced through "catalog:default" or "catalog:"
"default": {
"jest": "^29.6.1",
"redux": "^4.2.0",
"react-redux": "^8.0.0"
},

// Can be referenced through "catalog:react17"
"react17": {
"react": "^17.0.2",
"react-dom": "^17.0.2"
},

// Can be referenced through "catalog:react18"
"react18": {
"react": "^18.2.0",
"react-dom": "^18.2.0"
},

Choose a reason for hiding this comment

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

I imagine that most people will only need default, so for them, having to write a "default" object after "catalogs" will feel like a hassle.

Copy link
Member Author

@gluxon gluxon Aug 31, 2023

Choose a reason for hiding this comment

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

That's a good point. I do agree. Here's another potential config structure:

// package.json
{
  // Can be referenced through `catalog:default` or `catalog:`.
  "catalog": {
    "jest": "^29.6.1",
    "redux": "^4.2.0",
    "react-redux": "^8.0.0"
  },

  "namedCatalogs": {
    // Can be referenced through "catalog:react17"
    "react17": {
      "react": "^17.0.2",
      "react-dom": "^17.0.2"
    },

    // Can be referenced through "catalog:react18"
    "react18": {
      "react": "^18.2.0",
      "react-dom": "^18.2.0"
    }
  }
}

We could also name these defaultCatalog and catalogs, but under the idea that most users will just have a default catalog, I'd lean towards keeping that name shorter and just catalog.

Choose a reason for hiding this comment

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

We could also name these defaultCatalog and catalogs, but under the idea that most users will just have a default catalog, I'd lean towards keeping that name shorter and just catalog.

I'm fine with either way.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated the document for the catalog and namedCatalogs split here: b8b4527

Copy link
Member

Choose a reason for hiding this comment

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

You could also put everything to catalog like this

// package.json
{
  // Can be referenced through `catalog:default` or `catalog:`.
  "catalog": {
    "jest": "^29.6.1",
    "redux": "^4.2.0",
    "react-redux": "^8.0.0"

    // Can be referenced through "catalog:react17"
    "react17": {
      "react": "^17.0.2",
      "react-dom": "^17.0.2"
    },

    // Can be referenced through "catalog:react18"
    "react18": {
      "react": "^18.2.0",
      "react-dom": "^18.2.0"
    }
  }
}

Inspired by npm's overrides syntax. If you want to use the same name for the catalog group as a dependency name, you can use .. For instance:

{
  "catalog": {
    "jest": "^29.6.1",
    "redux": "^4.2.0",
    "react-redux": "^8.0.0"
    "react": {
      ".": "^17.0.2",
      "react-dom": "^17.0.2"
    }
}

Copy link
Member

Choose a reason for hiding this comment

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

I don't know how I feel about a separate file. I am not sure it is a great idea because we already have so many different ways to configure pnpm. But I will not reject the RFC if you decide to go with a separate file.

We might want to consider putting the new config file to a .pnpm directory.

Choose a reason for hiding this comment

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

Catalogs looks like a workspace feature. I think it should be in pnpm-workspace.yaml. This config file currently only has one top-level field called packages.

Copy link
Member Author

@gluxon gluxon Sep 7, 2023

Choose a reason for hiding this comment

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

From a solely end user experience perspective, it feels like there's no disagreement that a separate file (or reusing pnpm-workspace.yaml) results in a better configuration story? Since the contention is codebase complexity and reducing the multiple ways to configure pnpm, I definitely trust @zkochan to speak to that best.

I will wait for @zkochan's thoughts on reusing the pnpm-workspace.yaml file since I see a few others have reacted positively to that last suggestion. I personally want to provide the best user experience but maintaining a simpler codebase is also very important.

Copy link
Member

@zkochan zkochan Sep 7, 2023

Choose a reason for hiding this comment

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

We will definitely need to revise the different ways we configure pnpm. But pnpm-workspace.yaml already exists and as @KSXGitHub said the catalogs in the current state only make sense in a workspace, so I like this idea.

Copy link
Member Author

Choose a reason for hiding this comment

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

Perfect! Here's the commit modifying the RFC to specify pnpm-workspace.yaml. 2610503

I did switch back to catalog/namedCatalogs since it allows users to define a named catalog and a default catalog entry of the same name without the . syntax.

text/0001-catalogs.md Show resolved Hide resolved
@gluxon
Copy link
Member Author

gluxon commented Aug 31, 2023

Thanks for taking a look @mcmxcdev. Appreciate it.

So catalog: would replace the functionality from what react": "workspace:* does currently across multiple packages? Or I guess it's more advanced since you can specify multiple different catalogs for packages depending on different versions of a dependency.

That's right. Your take on this is accurate. My personal mental model is that this brings the nice ease-of-use ergonomics of workspace:* for external packages.

From what I have seen, https://github.com/JamieMason/syncpack which you also mentioned works quite well with keeping deps in sync, so I would consider this more "nice to have".

Are there any related GH issues in the pnpm repo that specifically wish for this functionality? Is there any real user interest in having this apart from some upvotes of this RFC?

Good question! Off the top of my head, there's a few discussions I've seen related to this.

Another angle I'm hoping catalogs to one day address is reduce lockfile merge conflicts, which is another frequently discussed topic. Some merge conflict resistant parts of this RFC were punted, but I'm hoping we can revisit them if there's a way to implement them without introducing significant complexity to the codebase.

Personally, since there are always limited resources in OSS, I would consider bug fixes and perf improvements to pnpm more important than this feature. But then again, I don't have to struggle with huge monorepos, so I don't feel the pain.

The resourcing question is definitely an important one to ask. My opinion is a bit biased from working heavily in huge monorepos day to day, but I do personally believe catalogs are a feature that uniquely boosts pnpm, and substantially. In the last few years, pnpm has shown that it scales much better than its alternatives.

One particular reason is that it encourages "correctness" when declaring dependencies. The common workaround for sharing dependencies across monorepo packages is to put them in the root package.json. However, pnpm's semi-strict node_modules structure encourages users to do the "right thing" and properly declare each used dependency in the specific workspace package package.json. This avoids a problem "Phantom Dependencies" problem I've found the Rush docs to explain particularly well.

But now we have a new problem with the same dependency needing to be written multiple times in many package.json files of a monorepo. I'm hoping this addresses that "new" problem. I'm expecting any user of pnpm that benefits from the semi-strict node_modules setup to also benefit from catalogs.

I would also prioritize bug fixes and perf improvements to pnpm over new features. I think this is one of the cases we should do both since the feature complements pnpm's strengths. It enhances a user story that's made pnpm popular over the last few years.

@superfreddialpad
Copy link

Is there any real user interest in having this apart from some upvotes of this RFC?

I'll note my voracious interest here. We're struggling with this problem badly in our monorepo, especially as we attempt to start deploying module federation. We're in the midst of cobbling together a solution which involves strictly pinning every workspace package's dependencies to one version (e.g. every single package/*/package.json uses [email protected] or whatever). It seems like it'll work for now, but I'm sure it's completely unsustainable long-term.

the catalog: proposal seems like it would suit our use case perfectly.

@jcoveney-anchorzero
Copy link

jcoveney-anchorzero commented Sep 8, 2023

I am not at all an expert on build systems, but I did just want to add my voice saying

Is there any real user interest in having this apart from some upvotes of this RFC?

we are using pnpm with a monorepo, and are quite happy, but we have been wanting this exact feature. I've been following it with great interesting. just wanted to reaffirm that this work doesn't exist in a vacuum, though I of course know OSS resources are limited and I respect whatever the pnpm team chooses to do. that said, I hope that this feature makes it into pnpm :) (it looks like it is going that direction!) keep up the great work everyone

@gluxon
Copy link
Member Author

gluxon commented Sep 8, 2023

Thank you @superfreddialpad and @jcoveney-anchorzero. It is really valuable hearing that we're creating a feature that will actually be useful. ❀️

@zkochan zkochan merged commit 37f8a7a into pnpm:main Sep 8, 2023
@evelant
Copy link

evelant commented Sep 8, 2023

I too have been following this with great interest. Maintaining consistent versions in a monorepo is a pain, this feature would be super valuable!

@jakeleventhal
Copy link

Where can we track the implementation of this?

@gluxon gluxon mentioned this pull request Sep 10, 2023
13 tasks
@gluxon gluxon deleted the 0001-workspace-consistent branch September 10, 2023 02:18
@gluxon
Copy link
Member Author

gluxon commented Sep 10, 2023

Thanks to everyone for their thoughts and feedback. The RFC is in a much better place from where it started. I'm glad to see this ratified and excited to begin working on it.

Where can we track the implementation of this?

Good question. I've just opened pnpm/pnpm#7072 to track the implementation side of this. I'll also comment on this pull request so anyone following the RFC will be notified of its availability in the future.

@jakeleventhal
Copy link

@gluxon with all the recent buzz, is there any way to incorporate bun into pnpm along with this RFC? or will they always be incompatible/competing?

@superfreddialpad
Copy link

@gluxon with all the recent buzz, is there any way to incorporate bun into pnpm along with this RFC? or will they always be incompatible/competing?

If I'm correct that you're referring to https://bun.sh/ β€” without regard to the merits of your particular suggestion nor those of bun in general β€” surely that's wildly out of scope for the current RFC?

Aside from the alleged recent buzz, is there anything about incorporating bun into pnpm that's germane to the catalog version specifier per se? I feel as though I must be missing something.

@jakeleventhal
Copy link

@superfreddialpad The point is that since bun is itself a package manager, it might make sense to keep the APIs in sync between the two. This feature doesn't exist in either ecosystem so now would be the time to make that happen.

@gluxon
Copy link
Member Author

gluxon commented Sep 27, 2023

@gluxon with all the recent buzz, is there any way to incorporate bun into pnpm along with this RFC? or will they always be incompatible/competing?

@jakeleventhal I'm definitely in favor of any cross-pollination of ideas between package managers. (Zoltan in the past contributed to npm's isolated mode RFC as a prior example.) Implementing this in pnpm would be the first priority, but I'd personally be excited to see this feature in Bun (or other package managers) and happy to lend help if asked.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet