👋 yarn@1: align some DCR deps
#9565
Merged
Conversation
This file contains hidden or 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
|
Size Change: 0 B Total Size: 700 kB ℹ️ View Unchanged
|
sndrs
force-pushed
the
sndrs/remove-yarn@1-pt3
branch
from
049da2f to
5fb9102
Compare
sndrs
force-pushed
the
sndrs/remove-yarn@1-pt3
branch
from
5fb9102 to
94761fa
Compare
mxdvl
reviewed
Nov 21, 2023
| @@ -41,8 +41,7 @@ | |||
| "lint-staged": "13.2.1", | |||
| "npm-run-all": "^4.1.5", | |||
| "prettier": "3.0.3", | |||
| "tslib": "2.5.3", | |||
| "typescript": "5.1.3" | |||
There was a problem hiding this comment.
Does TypeScript need to be removed from the top-level package? Does this not increase the risk of mismatched TS versions? And why would tslib be left there?
There was a problem hiding this comment.
Does TypeScript need to be removed from the top-level package? Does this not increase the risk of mismatched TS versions?
So nothing in the root actually uses TypeScript.
Right now, this root dependency is only there to provide DCR with TypeScript (because DCR doesn't declare a dependency on it), but not AR (which has a declared dependency an earlier version and is not part of the workspace).
As you suggest, there are two ways of providing common deps like this - installing them once in the root, or by requiring projects in the workspace to be explicit.
In csnx, we've found it a lot more straightforward to have projects declare their own dependencies (the ones declared here are for deps that are also used by the root, so we know are present – this set up also feels a bit weird to me).
But if you chose to provide them all from the root, then you need to know exactly which dependencies all projects have, manually decide which of those make sense to pull up to the root, and then keep on top of whether those projects continue to need them (since they're not declared anywhere, how do you know?).
You are also bound to change all projects simultaneously, which can make a simple bump in one package a far more complicated change as you fix a load of issues in unrelated packages.
The trade off is needing to manually keep common dependencies in sync (note these are not peerDeps, just common deps – as long as packages declare peerDependencies, this should be safe).
Moving common dependency declarations to the root does guarantee version parity across projects, but at the cost of flexibility and an increased maintenance overhead (are the assumptions in the root still correct?).
It's also worth considering that AR and DCR currently use different versions of TypeScript without issue. In CSNX we have linting scripts, for example, in the root that run across all projects that we therefore want to only use the root version of eslint.
I think it's tempting to think that version parity is always preferable over drift, but the cost of forcing it is high and where it's not a direct a benefit (i.e. the code can run) it's not proven to be worth it, at least in our experience of CSNX.
There was a problem hiding this comment.
And why would
tslibbe left there?
it's a peer dep of @guardian/prettier
mxdvl
approved these changes
Nov 21, 2023
There was a problem hiding this comment.
Thanks for a very clearly explained rationale in https://github.com/guardian/dotcom-rendering/pull/9565/files#r1400398584
I’d suggest bringing this up in the PR description, as it’s one of the best writeup on the distinction between root and workspace dependencies I’ve seen! 👏
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.
What does this change?
aligns some root and dcr deps, making the workspace more explicit and apps less dependent on yarn 1's hoisting
edit: a wider discussion about this took place in the comments, including a breakdown of the rationale (which should have been in the description originally really) – see #9565 (review) for more info
Why?
prep for moving away from yarn 1