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

build(all): switch from yarn to pnpm #146

Merged
merged 9 commits into from
Jan 20, 2021
Merged

build(all): switch from yarn to pnpm #146

merged 9 commits into from
Jan 20, 2021

Conversation

eventualbuddha
Copy link
Collaborator

@eventualbuddha eventualbuddha commented Jan 13, 2021

I tried to get all the apps and libraries under a single yarn workspace and found I could not do it. This message describes the difficulties I had. Closes #75.

"There might be a problem with the project dependency tree."

react-scripts does some preflight checks to ensure that there isn't another resolvable copy of some of the packages it needs that might conflict with the versions it wants. When it finds one, it bails with an error message.

Trying to put multiple react-scripts-using projects into the same yarn workspace means that yarn will hoist shared dependencies, such as babel-eslint, to the root. Thus, these packages become visible to the preflight check and it aborts. To work around this, you can use the nohoist option to tell yarn which packages not to hoist. This sort of worked, but there were subtle issues remaining.

Versions changing when adding to the workspace

I got BMD and ballot-encoder into the workspace together, but when I added election-manager it broke BMD. I still don't know exactly why, but I believe something changed the versions of some packages that BMD was using in a way that caused the tests to fail.

Incompatible type declarations

Sometimes multiple packages reference the same types, i.e. for a mutual dependency. However, sometimes they'd end up using different (and subtly incompatible) versions of the same @types/ package. When this happened, the best recourse was to use another yarn workspaces feature: resolutions. This allows you to configure what version packages should resolve to. This fixed most of the type incompatibility issues, but I suspect it caused another issue.

yarn resolved an older copy than requested

I noticed some crashes due to an API incompatibility. Specifically, one of the @babel/ packages required another one in the ^7.12.0 range, but yarn hooked it up with 7.8.6. Why? I don't know. Perhaps it was the resolutions stuff. I was only able to work around this by finding the entry in the yarn.lock file, deleting it, and re-running yarn.

Trying out alternatives

I tried both npm v7 and pnpm. npm v7 has support for workspaces similar to yarn, but does not yet have support for configuring hoisting behavior. Lacking hoisting control meant it hoisted everything it could, which made react-scripts angry. Thus I couldn't use npm v7.

Previous versions of pnpm did not hoist anything, and still was able to share dependencies by symlinking. Though it does hoist by default now, that behavior can be turned off. So I added hoist = false to .npmrc and then started trying to run everything to determine what would break without hoisting. It turns out that a good number of packages have implicit dependencies that hoisting allowed them to access, but that could not be accessed in a non-hoisted node_modules layout.

pnpm has a hook called readPackage that allows altering the package data for any npm package using any custom logic you want. This allows us to act as if these packages with implicit dependencies actually had those dependencies listed explicitly. In my testing, this has yielded a good balance of correctness and functionality.

So pnpm seems to work and I think is flexible enough to meet our needs going forward.

@eventualbuddha
Copy link
Collaborator Author

Here's a quick summary of differences I've discovered through usage in the pnpm and yarn CLIs:

Command yarn pnpm
Install dependencies yarn pnpm i
Add react to dependencies yarn add react pnpm install react
Add eslint to devDependencies yarn add -D eslint pnpm install -D eslint
Run script test yarn test pnpm test
Run script lint with args yarn lint --fix pnpm lint -- --fix
Run package binary tsc yarn tsc --build pnpx tsc --build

@eventualbuddha eventualbuddha force-pushed the build/all/pnpm branch 3 times, most recently from 2e14d90 to c7353f7 Compare January 14, 2021 19:39
Copy link
Contributor

@benadida benadida left a comment

Choose a reason for hiding this comment

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

  • we need a basic README at the top level now, I think
  • should include how to install pnpm the right way.
  • ignore .eslintcache files.
  • Election Manager not working with make run

@@ -6,12 +6,10 @@ Interprets VotingWorks ballots marked by hand and scanned into images.

```sh
# Local install for API usage.
$ yarn add @votingworks/hmpb-interpreter
$ npm install @votingworks/hmpb-interpreter # or, with npm
$ npm install @votingworks/hmpb-interpreter
Copy link
Contributor

Choose a reason for hiding this comment

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

npm, not pnpm? When to use which?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is for "external" consumers of the published library, so I think it makes sense to use the canonical package manager in the docs here.

@eventualbuddha
Copy link
Collaborator Author

eventualbuddha commented Jan 14, 2021

  • we need a basic README at the top level now, I think

Got a really basic one up (preview)

  • should include how to install pnpm the right way.

I put this in the README, though it's a little buried.

  • ignore .eslintcache files.

Done.

  • Election Manager not working with make run

Should be fixed.

@benadida benadida self-requested a review January 15, 2021 01:32
@eventualbuddha eventualbuddha force-pushed the build/all/pnpm branch 2 times, most recently from 7f3ab22 to f1437a2 Compare January 19, 2021 18:33
@carolinemodic
Copy link
Contributor

Played around a bit and things mostly worked as expected except election-manager. When I run yarn start in apps/election-manager I get:

src/AppRoot.tsx
  Line 67:10:  Replace `⏎····electionDefinition,⏎····setElectionDefinition,⏎··]·=·useState<ElectionDefinition` with `electionDefinition,·setElectionDefinition]·=·useState<⏎····ElectionDefinition⏎··`  prettier/prettier

src/lib/votecounting.ts
  Line 458:38:  Replace `⏎····election.contests⏎··).reduce<ContestTallyMetaDictionary` with `election.contests).reduce<⏎····ContestTallyMetaDictionary⏎··`  prettier/prettier

When I make run instead it seems to run ok but when I launch kiosk-browser to localhost:3000 it doesn't load anything.

@eventualbuddha
Copy link
Collaborator Author

Thanks @carolinemodic, I think 3c11e45 fixes it. Try again?

I tried to get all the apps and libraries under a single yarn workspace and found I could not do it. This message describes the difficulties I had.

`react-scripts` does some preflight checks to ensure that there isn't another resolvable copy of some of the packages it needs that might conflict with the versions it wants. When it finds one, [it bails with an error message](facebook/create-react-app#4296).

Trying to put multiple `react-scripts`-using projects into the same yarn workspace means that yarn will hoist shared dependencies, such as `babel-eslint`, to the root. Thus, these packages become visible to the preflight check and it aborts. To work around this, you can use the [`nohoist`](https://classic.yarnpkg.com/blog/2018/02/15/nohoist/) option to tell yarn which packages not to hoist. This sort of worked, but there were subtle issues remaining.

I got BMD and ballot-encoder into the workspace together, but when I added election-manager [it broke BMD](#139). I still don't know exactly why, but I believe something changed the versions of some packages that BMD was using in a way that caused the tests to fail.

Sometimes multiple packages reference the same types, i.e. for a mutual dependency. However, sometimes they'd end up using different (and subtly incompatible) versions of the same `@types/` package. When this happened, the best recourse was to use another yarn workspaces feature: `resolutions`. This allows you to configure what version packages should resolve to. This fixed most of the type incompatibility issues, but I suspect it caused another issue.

I noticed some crashes due to an API incompatibility. Specifically, one of the `@babel/` packages required another one in the `^7.12.0` range, but yarn hooked it up with `7.8.6`. Why? I don't know. Perhaps it was the `resolutions` stuff. I was only able to work around this by finding the entry in the `yarn.lock` file, deleting it, and re-running `yarn`.

I tried both npm v7 and pnpm. npm v7 has support for workspaces similar to yarn, but does not yet have support for configuring hoisting behavior. Lacking hoisting control meant it hoisted everything it could, which made `react-scripts` angry. Thus I couldn't use npm v7.

Previous versions of pnpm did not hoist anything, and still was able to share dependencies by symlinking. Though it does hoist by default now, that behavior can be turned off. So I added `hoist = false` to `.npmrc` and then started trying to run everything to determine what would break without hoisting. It turns out that a good number of packages have implicit dependencies that hoisting allowed them to access, but that could not be accessed in a non-hoisted `node_modules` layout.

pnpm has a hook called `readPackage` that allows altering the package data for any npm package using any custom logic you want. This allows us to act as if these packages with implicit dependencies actually had those dependencies listed explicitly. In my testing, this has yielded a good balance of correctness and functionality.

So pnpm seems to work and I think is flexible enough to meet our needs going forward.
This also updates `bas` to use types from `@votingworks/ballot-encoder`.
We changed it to string batch IDs a long time ago, but this only became a problem because an updated version of `@types/express` has better typing for request params. Previously we were passing `any`, now it's typed as `string`.
This required pinning typescript to v4.0.5 due to microsoft/TypeScript#42350. I don't yet understand why this bug manifested with pnpm and not with yarn, but it may get fixed in TS 4.3.0 or earlier.
These were in module-scan but really belong as their own packages.
@carolinemodic
Copy link
Contributor

Hmm I was still getting the error but then did a rm -rf of node_modules and redid pnpm install and now election-manager is running fine so... not sure if the changes in 3c11e45 changed anything for me but it does seem to work ok now!

Something broke with the node-sqlite3 package in the cache, so let's try again.
Copy link
Contributor

@beausmith beausmith left a comment

Choose a reason for hiding this comment

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

Running Locally. LGTM.

@eventualbuddha eventualbuddha merged commit 0409a68 into main Jan 20, 2021
@eventualbuddha eventualbuddha deleted the build/all/pnpm branch January 20, 2021 18:49
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.

figure out how to share dependencies in monorepo
4 participants