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

Fix/mass deduplication 2 #48752

Merged
merged 5 commits into from
Jan 11, 2021
Merged

Fix/mass deduplication 2 #48752

merged 5 commits into from
Jan 11, 2021

Conversation

scinos
Copy link
Contributor

@scinos scinos commented Jan 11, 2021

Changes proposed in this Pull Request

Aggregation of PR's deduplicating packages:

Each PR has been reviewed and approved.

@matticbot
Copy link
Contributor

@matticbot
Copy link
Contributor

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

@scinos scinos merged commit 25e2a07 into trunk Jan 11, 2021
@scinos scinos deleted the fix/mass-deduplication-2 branch January 11, 2021 07:07
@@ -8760,7 +8690,7 @@ [email protected]:
node-releases "^1.1.52"
pkg-up "^3.1.0"

browserslist@^4.0.0, browserslist@^4.11.1, browserslist@^4.12.0, browserslist@^4.6.4, browserslist@^4.8.0, browserslist@^4.8.2, browserslist@^4.8.5, browserslist@^4.9.1:
Copy link
Member

Choose a reason for hiding this comment

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

@scinos browserslist could use further deduplication here. Now it has two versions installed, 4.14.0 and 4.14.7. caniuse-lite and autoprefixer are in a similar situation.

Noticed this when resolving lockfile conflicts with #48729.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure it can be safely deduplicated, as our tree requires two different non-overlapping ranges of browserlist:

Most likely react-dev-utils will work fine with [email protected], but I rather not override the version specified by a project without first understanding why react-dev-utils uses a fixed version and maybe asking them to switch to ranges.

Looks like this "explicit duplication" is very common in our repo: a basic grep tells me we have 548 packages with two or more versions, while yarn-deduplicate --list only detects 136 packages candidates to be deduplicated.

Copy link
Member

Choose a reason for hiding this comment

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

I see, there are actually three records for browserslist in the lockfile. I haven't noticed the first, pinned one before:

  1. [email protected] -> 4.10.0. That's a pinned version, nothing can be done about it.
  2. browserslist@^4.0.0 + 5 other specs -> 4.14.0. All of them use carets (^) and should be flexible.
  3. browserslist@^4.14.5 -> 4.14.7. This one could be deduplicated and merged into the big list above. And the whole list should deduplicate into the 4.14.7 version.

I wonder why yarn-deduplicate didn't find that records 2 and 3 can be merged into one? Maybe it got confused by record 1 somehow?

Copy link
Contributor Author

@scinos scinos Jan 11, 2021

Choose a reason for hiding this comment

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

Very interesting! Maybe the extra record was added after I run the initial yarn-deduplicate for browserslist? Maybe it is a bug in yarn-deduplicate

It seems to detect it fine now in trunk:

Package "browserslist" wants ^4.0.0 and could get 4.14.7, but got 4.14.0
Package "browserslist" wants ^4.12.0 and could get 4.14.7, but got 4.14.0
Package "browserslist" wants ^4.6.4 and could get 4.14.7, but got 4.14.0
Package "browserslist" wants ^4.8.2 and could get 4.14.7, but got 4.14.0
Package "browserslist" wants ^4.8.5 and could get 4.14.7, but got 4.14.0
Package "browserslist" wants ^4.9.1 and could get 4.14.7, but got 4.14.0

And in fact, npx yarn-deduplicate --packages browserslist moves all specs using carets to one single version, 4.14.7

Copy link
Contributor Author

@scinos scinos Jan 11, 2021

Choose a reason for hiding this comment

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

Actually it looks like a bad merge: the original PR correctly deduplicates all versions: https://github.com/Automattic/wp-calypso/pull/48594/files.

Edit: uh. It was re-added in f4121b7

Copy link
Member

Choose a reason for hiding this comment

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

Interesting. In that autoprefixer deduplication, some browserslists semvers stopped being requested, so it makes sense that the browserslists record would be modified (removing an item from the list). But why it was split into two? Looking at the diff, that was not necessary at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did those in two different PRs (actually I have a script that does each dedupe in an individual PR), but I merge them into this branch/PR to avoid creating too much noise in the deployment queue.

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.

3 participants