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

Dependencies: Deduplicate @wordpress/ packages #17559

Merged
merged 1 commit into from
Oct 22, 2020

Conversation

ockham
Copy link
Contributor

@ockham ockham commented Oct 21, 2020

Follow-up to #16845 (comment), where @jsnajdr suggested de-duplicating dependencies in yarn.lock, which had been blocking #16845.

Changes proposed in this Pull Request:

Deduplicate dependencies in yarn.lock by using yarn-deduplicate

npx yarn-deduplicate --scopes @wordpress

This is the first time I'm using this, hope I'm doing it right 🤞

Jetpack product discussion

N/A

Does this pull request change what data or activity we track or use?

No

Testing instructions:

Verify that things still work as before (check CI, do sufficient smoke testing).

Proposed changelog entry for your changes:

Deduplicate @wordpress/ dependencies

@ockham ockham added [Status] Needs Review To request a review from Crew. Label will be renamed soon. [Type] Janitorial labels Oct 21, 2020
@ockham ockham requested review from jeherve, jsnajdr, scinos, brbrr and a team October 21, 2020 11:03
@ockham ockham self-assigned this Oct 21, 2020
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello ockham! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer and confirm D51560-code works as expected before merging this PR. Once this PR is merged, please commit the changes to WP.com. Thank you!
This revision will be updated with each commit to this PR

@ockham
Copy link
Contributor Author

ockham commented Oct 21, 2020

Hold on, I'll try just deduplicating @wordpress/ packages for now...

@ockham ockham added [Status] In Progress and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Oct 21, 2020
@ockham ockham changed the title Dependencies: Deduplicate Dependencies: Deduplicate @wordpress/ packages Oct 21, 2020
@ockham ockham force-pushed the update/deduplicate-dependencies branch from 55f4036 to 26a81d2 Compare October 21, 2020 11:08
@ockham ockham added [Status] Needs Review To request a review from Crew. Label will be renamed soon. and removed [Status] In Progress labels Oct 21, 2020
@jetpackbot
Copy link

jetpackbot commented Oct 21, 2020

Scheduled Jetpack release: November 10, 2020.
Scheduled code freeze: November 3, 2020

E2E results is available here (for debugging purposes): https://jetpack-e2e-dashboard.herokuapp.com/pr-17559

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Generated by 🚫 dangerJS against 346d5db

@ockham ockham force-pushed the update/deduplicate-dependencies branch from 26a81d2 to 346d5db Compare October 22, 2020 12:53
@jeherve jeherve added this to the 9.1 milestone Oct 22, 2020
@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! Build and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Oct 22, 2020
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

This looks good. Let's merge it right now so it gets tested by everyone in the next few weeks.

How can we ensure that duplications don't happen again next time we update packages?

@kraftbj kraftbj merged commit 7b2a54a into master Oct 22, 2020
@kraftbj kraftbj deleted the update/deduplicate-dependencies branch October 22, 2020 22:25
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Oct 22, 2020
@kraftbj
Copy link
Contributor

kraftbj commented Oct 22, 2020

r215724-wpcom

@ockham
Copy link
Contributor Author

ockham commented Oct 23, 2020

This looks good. Let's merge it right now so it gets tested by everyone in the next few weeks.

How can we ensure that duplications don't happen again next time we update packages?

I'm not entirely sure TBH. @scinos might know 😄

@scinos
Copy link

scinos commented Oct 23, 2020

How can we ensure that duplications don't happen again next time we update packages?

I'm not entirely sure TBH. @scinos might know 😄

AFAIK it is not possible. By design, yarn won't dedupe existing packages because technically is a risky operation (eg: some code path was getting lib@1.0.0 before and now it gets lib@1.0.1), which they don't want to do.

If we can achieve total deduplication then we could add a new CI job to run npx yarn-deduplicate --fail, which will fail if a new duplication is introduced. That will at least remind us to run yarn-deduplicate when we bump dependencies.

@jsnajdr
Copy link
Member

jsnajdr commented Oct 23, 2020

How can we ensure that duplications don't happen again next time we update packages?

We plan to re-declare many dependencies in the Gutenberg monorepo as peer dependencies: WordPress/gutenberg#8981 (comment) The proposal was discussed in a core-js chat this week and will most likely be implemented.

That doesn't solve everything, but will prevent yarn/npm from installing duplicate packages where it's obviously undesired, and will make them issue a warning instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants