refactor(all): link internal apps to libs #147
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
eventualbuddha
force-pushed
the
refactor/all/link-libs
branch
3 times, most recently
from
9899256 to
ef0022a
Compare
eventualbuddha
force-pushed
the
build/all/pnpm
branch
from
a3fb450 to
34eeef8
Compare
eventualbuddha
force-pushed
the
refactor/all/link-libs
branch
from
ef0022a to
09faa4b
Compare
eventualbuddha
force-pushed
the
build/all/pnpm
branch
2 times, most recently
from
2e14d90 to
c7353f7
Compare
eventualbuddha
force-pushed
the
refactor/all/link-libs
branch
from
09faa4b to
7a1acce
Compare
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.
eventualbuddha
force-pushed
the
refactor/all/link-libs
branch
from
7a1acce to
73cc605
Compare
|
Consolidating this into #146. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This also updates
basto use types from@votingworks/ballot-encoder.