Skip to content
This repository has been archived by the owner on Jun 9, 2023. It is now read-only.

chore: switch to npm workspaces #869

Merged

Conversation

ojeytonwilliams
Copy link
Contributor

  • I have read Chapter's contributing guidelines.
  • My pull request has a descriptive title (not a vague title like Update README.md).
  • My pull request targets the main branch of Chapter.

This isn't a huge deal, since we only have three packages, but workspaces are quite nice. They cut down on duplication and, as a result, speed up installation. Plus, we don't need our own code for installing all the packages.

@gitpod-io
Copy link

gitpod-io bot commented Jan 20, 2022

@ghost
Copy link

ghost commented Jan 20, 2022

CodeSee Review Map:

Review these changes using an interactive CodeSee Map

Review in an interactive map

View more CodeSee Maps

Legend

CodeSee Map Legend

@ojeytonwilliams ojeytonwilliams force-pushed the chore/use-workspaces branch 2 times, most recently from 7df2d04 to dec565d Compare January 21, 2022 11:35
@ojeytonwilliams
Copy link
Contributor Author

@gikf @Ravichandra-C does this all make sense? You'll be the most directly impacted in the immediate future, so if some of this seems silly let me know!

@gikf
Copy link
Member

gikf commented Jan 21, 2022

@ojeytonwilliams I couldn't say, I'm not too familiar with how this should be configured.

@ojeytonwilliams
Copy link
Contributor Author

Not to worry, @gikf

Not much should change, honestly. The only real quirk I ran into is that prisma doesn't automatically prisma generate if you use workspaces, so I had to do that manually.

Anyways, it's not urgent, so I'll give Ravi a chance to see it before I merge.

@Ravichandra-C
Copy link
Contributor

@ojeytonwilliams , I'm also not very familiar with this.

@ojeytonwilliams
Copy link
Contributor Author

Okay, the idea goes like this. Rather than having three packages, (root, client and server) all with independent package-locks, we treat the client and server as workspaces and have a single package-lock that governs all three.

This dramatically cuts down on package duplication and, because npm understands workspaces, we no longer need to handle their installation. i.e. we don't need to use npx recursive-install, lerna or (as we currently do) have our own code for installing the client and server. Instead, running npm i does it all.

In practical terms, you have to use npm i -w=client new-package if you want to install a new package in the client (and -w=server for the server, naturally). You'll know that something went wrong if a new package-lock appears in a PR.

@allella
Copy link
Contributor

allella commented Jan 24, 2022

@ojeytonwilliams does this npm workspaces change anything about our dependencies or minimum versions of node or npm?

Is all that changes is some of the npm i commands?

@ojeytonwilliams
Copy link
Contributor Author

@ojeytonwilliams does this npm workspaces change anything about our dependencies or minimum versions of node or
npm?

No, they're unchanged. Workspaces need npm 7, but we've required 8 for a while now.

Is all that changes is some of the npm i commands?

Pretty much. Anything you'd have previously done via cd package && npm... you'll need to do via npm -w=package.

@allella
Copy link
Contributor

allella commented Jan 26, 2022

It sounds to me like we can update the docs and go with workspaces if it's not going to confuse or bother the casual contributor.

@ojeytonwilliams
Copy link
Contributor Author

@allella I think so. I've add a little to the docs in this PR, but I'm going to merge as soon as the CI passes. Just because otherwise renovate might merge something and rebasing is a nuisance.

@ojeytonwilliams ojeytonwilliams merged commit 7e830b8 into freeCodeCamp:main Jan 26, 2022
@ojeytonwilliams ojeytonwilliams deleted the chore/use-workspaces branch January 26, 2022 17:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants