[rush] Support workspaces within PNPM #1897
Conversation
D4N14L
requested review from
apostolisms,
halfnibble,
iclanton,
octogonz and
sachinjoseph
as code owners
iclanton
changed the title
| public async doInstall(): Promise<void> { | ||
| // Workspaces do not support the noLink option, so throw if this is passed | ||
| if (this.options.noLink) { | ||
| console.log(); |
There was a problem hiding this comment.
I just saw somewhere else in the code where we use os.EOL, which might be a good alternative to the empty log().
|
|
||
| // Workspace ranges are a feature from PNPM and Yarn. Set the version specifier | ||
| // to the trimmed version range. | ||
| if (versionSpecifier.startsWith('workspace:')) { |
There was a problem hiding this comment.
The workspace: specifier is an interesting feature. Rush's equivalent is:
- If the dependency is SemVer-compatible with a Rush project version, it gets locally linked.
- You can disable that using
cyclicDependencyProjects.
Issue #547 proposes to report an error if a dependency is NOT SemVer-compatible and NOT in cyclicDependencyProjects. And to rename it to installedDependencyProjects.
If we implemented that, then adding to installedDependencyProjects would be equivalent to NOT using the workspace: specifier. We could potentially retire the cyclicDependencyProjects/installedDependencyProjects feature entirely and just use workspace: specifier.
But there's one downside of workspace: In a monorepo, you almost always want to locally link projects. Seems like it would be very common beginner mistake to forget to add workspace: and then be confused why Rush is trying to install your package from the registry.
Maybe you guys could debate this and update #547 with your decision.
There was a problem hiding this comment.
Correct, that would be confusing :) I actually have a remedy for that problem in the workspace manager. When encountering this scenario, we check the package.json versions using the same logic for local-linking. If we determine that it would have been locally linked, then we modify the package.json file to specify workspace:* and write a warning to the console. If the version range doesn't match the local project, we validate that it's specified in cyclicDependencyProjects. If it is still not found, then we throw an error stating that it needs to either be added to cyclicDependencyProjects, or needs to target the local version.
In this way, the only time that a local package can be specified to be downloaded from the registry instead of locally linked would be to specify a normal version range in the package.json, ex ~1.2.3, and add it to the cyclicDependencyProjects field.
Devs can also manually specify their own workspace:~1.2.3 range for a local project. However, we will explicitly allow PNPM to handle resolution of that version range and let PNPM fail if it cannot be found.
There was a problem hiding this comment.
If we determine that it would have been locally linked, then we modify the package.json file to specify
workspace:*and write a warning to the console.
If you make it an error instead of a warning, I think you could resolve #547 with your PR.
But then cyclicDependencyProjects is not just for cyclic dependencies any more; it is for any situation where you want to install a previously published release. So it would make sense to rename the setting to something like installedDependencyProjects or nonlinkedProjects or somesuch.
To avoid breaking compatibility, we could document cyclicDependencyProjects as deprecated (removed in the next major release) and introduce the new setting alongside the old one. We could also prohibit cyclicDependencyProjects when pnpmOptions.useWorkspaces = true
|
I made some progress with this review, but I did not get to:
If you aren't in a hurry to merge the PR, I'd be interested to do those things when I get some more time. |
I'm going to create a new PR with feedback from this included. Too much has changed on master and the merge is a mess. So I'm just going to start from scratch and bring my changes in there |
"Prototype branches" are a great practice that people really should use more often. Once you work through the problems and have a working solution, it seems like a big hassle to go back and redo everything in a new branch. But it turns out to be surprisingly easier the second time around, and revisiting the work sometimes provides useful insights. You might also find a way to break up the work into multiple PRs, which can make the code review go faster as well. |
|
Closing out in favor of the newer PR here: #1938 |
This PR is to add workspaces features to PNPM (as requested in a few issues, but currently tracked in #1887 .
Workspaces is supported with PNPM in Rush in the following way:
pnpmOptions.useWorkspacestotrueinrush.json. This setting defaults tofalsecommon/tempfolder. As such, pnpm >= 4.14.3 is required since a change was needed to support workspaces outside of the directory tree in which the install is run (see related PNPM change)rush update --fullto update and re-write the lockfile to be compatible with workspacespnpm-workspace.yamlfile is created in thecommon/tempfolder, directly referencing all packages specified inrush.jsonpackage.jsonfiles are checked to find local project references, and the version is replaced withworkspace:*in order to be compatible with workspace ranges. If a local package is specified but the version is not compatible, then we will check if it's a cyclic dependency. If it is not specified as one, we will fail the install.--link-workspace-packages=false. This is done so that workspace packages are explicit within RushThings that will be improved in the future:
common-versions.jsonis not fully supported. The associatedpackage.jsonis still generated incommon/tempas it was before, however since each workspace package is now a separate project (instead of one large project), PNPM does not resolve package versions for common versions as it has in the past. Future work to shim the common versions usingpnpmfile.jswill need to be donerush install --to ...orrush install --from ...is needed before we can use the filtering feature of PNPM workspace installs