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

[rush] Support PNPM workspaces #1938

Merged
merged 39 commits into from
Jul 3, 2020
Merged

Conversation

D4N14L
Copy link
Member

@D4N14L D4N14L commented Jun 17, 2020

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:

  • The feature is enabled by setting pnpmOptions.useWorkspaces to true in rush.json. This setting defaults to false
  • The install is still performed from the root of the common/temp folder. 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)
  • On first run, if another version of a PNPM lockfile already exists, then we force a rush update --full to update and re-write the lockfile to be compatible with workspaces
  • A pnpm-workspace.yaml file is created in the common/temp folder, directly referencing all packages specified in rush.json
  • All package.json files are checked to find local project references, and the version is replaced with workspace:* 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.
  • PNPM is run in recursive mode, with --link-workspace-packages=false. This is done so that workspace packages are explicit within Rush
  • Preferred versions support is provided via a shim pnpmfile implementation that substitutes out package versions for preferred versions, if applicable. This is done because packages are no longer able to be hoisted up and used as a common version

Things that will be improved in the future:

  • Future work will be required to support rush install --to ... or rush install --from ... is needed before we can use the filtering feature of PNPM workspace installs

@D4N14L
Copy link
Member Author

D4N14L commented Jun 19, 2020

Note: currently working on changes to rush add, rush version, and rush publish to support workspaces, and exploring automated workspace range version bumping (for when a workspace range other than '*' is specified).

@octogonz
Copy link
Collaborator

octogonz commented Jul 1, 2020

I encountered that issue again where rush build failed after a previous enlistment was converted to use workspaces. The error message involves a broken symlink.

BTW it took me a while to realize that with workspaces, it is correct behavior for common\temp\node_modules to be mostly empty. (That misunderstanding is why I previously thought that rush install --purge wasn't resolving the problem.)

I am still not able to pin down a repro, but this problem definitely seems to be caused by some previous state being left over from a non-workspaces installation.

@D4N14L Should we add a field like "workspaces": true to last-install.flag? That would ensure that everything gets purged when switching from non-workspaces --> workspaces.

@D4N14L
Copy link
Member Author

D4N14L commented Jul 1, 2020

@D4N14L Should we add a field like "workspaces": true to last-install.flag? That would ensure that everything gets purged when switching from non-workspaces --> workspaces.

Agree on that. I'll add that in there.

apps/rush-lib/src/logic/DependencySpecifier.ts Outdated Show resolved Hide resolved
apps/rush-lib/src/logic/DependencySpecifier.ts Outdated Show resolved Hide resolved
apps/rush-lib/src/logic/pnpm/PnpmfileShim.ts Outdated Show resolved Hide resolved
@zkochan
Copy link
Contributor

zkochan commented Jul 8, 2020

So how can I use Rush with pnpm? What is the easiest way to convert a repo?

@arri-cc
Copy link
Contributor

arri-cc commented Jul 8, 2020

@zkochan I'm no expert, but I've switched between package managers a few times to test different outcomes. it came down to

  1. rush purge
  2. removing the package manager specific files from the common/config/rush folder (i.e. pnpm-lock.yaml, pnpmfile.js)
  3. updating the rush.json options to use a different package manager,
  4. run rush update.

@octogonz
Copy link
Collaborator

octogonz commented Jul 8, 2020

So how can I use Rush with pnpm? What is the easiest way to convert a repo?

This feature is still "experimental" and doesn't have website docs yet. But basically you follow the normal steps but set "useWorkspaces": true in rush.json.

CC @D4N14L

@michens
Copy link

michens commented Jul 8, 2020

I really wish the design for this would enable using a pnpm-workspace.yaml at the root instead of in common/temp.
pnpm recursive gives a lot of functionality for script running that would work well alongside rush.

@D4N14L
Copy link
Member Author

D4N14L commented Jul 9, 2020

@orokanasaru so this has come up a few times, and there's been some discussion. There's essentially two options that we've narrowed it down to:

  • Generate all required files and place them in the root of the repo, such that pnpm commands can be run natively
  • Create a utility package that can be installed (or a command built into Rush, ex. rush pnpm ...) to forward pnpm commands to be run in the common/temp directory

For the first option, the way I see it working would be to add pnpm-lock.yaml and pnpm-workspace.yaml to the .gitignore, and use the root dir to hold these files instead of the temp dir as we do currently. This would mean that rush install or rush update would need to be run at least once in order to get these files in place. Additionally, the path to the pnpmfile.js in common/temp would always need to be specified for installs (we also create a pnpmfile shim to add support for common versions, which further complicates things), and we would need to manually modify the checked-in pnpm-lock.yaml before dropping it in the root in order to have relative paths that are correct for a root-based install. So there's a few issues with this approach, likely more that I haven't thought of.

The second option seems more appealing to me. Yes, you would need to call the commands through a 'wrapper' package of sorts, but all this package would do is run your pnpm commands in the common/temp directory, possibly modifying some path-based arguments. It would also allow us to have some ability to notify devs if a command that is being run may not be compatible, but for the most part it would keep out of the way. And, since all commands would be run in the common/temp directory, any changes to files would be removed after running rush install/rush update, ensuring that the Rush repo itself is in a good state.

Thoughts?

@michens
Copy link

michens commented Jul 9, 2020

@D4N14L I agree that there would be issues putting everything at the root and that is probably not the ideal solution.

It seems like figuring out step #2 for adding custom parameters to commands would provide users quite a bit of flexibility in defining their own wrappers.

As an example, our repo already has a rush audit command that runs pnpm audit from common/temp. However, I can't write a similar rush why command without adding thousands of packages as alternative choices.

Rush providing a wrapper package (and hopefully a corresponding API in rushlib) to do some translation and analysis of commands to make this simpler and safer would be a nice bonus.

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.

7 participants