Skip to content

chore(deps): bump CLI to 9.1.3 and Metro to 0.72.3 - #34803

Closed
kelset wants to merge 3 commits into
mainfrom
kelset/bump-metro-and-cli
Closed

chore(deps): bump CLI to 9.1.3 and Metro to 0.72.3#34803
kelset wants to merge 3 commits into
mainfrom
kelset/bump-metro-and-cli

Conversation

@kelset

@kelsetkelset commented Sep 27, 2022

Copy link
Copy Markdown
Contributor

Summary

This PR bumps the dep version of Metro and the RN CLI to latest, and realigns them to avoid the issue we currently have in 0.70: #34714 (this commit will be cherry-picked there)

Also, it pins it all down to precise version. See comments for reasoning.

While at it, I gave a cleanup pass to the yarn.lock with yarn deduplicate.

Changelog

[General] [Changed] - bump CLI to 9.1.3 and Metro to 0.72.3

Test Plan

CI is green

@kelset
kelset requested a review from hramos as a code ownerSeptember 27, 2022 14:36
@facebook-github-botfacebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Contributor A React Native contributor. p: Microsoft Partner: Microsoft Partner labels Sep 27, 2022
@github-actions

github-actionsBot commented Sep 27, 2022

Copy link
Copy Markdown
Warnings
⚠️🔒 package.json - Changes were made to package.json. This will require a manual import by a Facebook employee.

Generated by 🚫 dangerJS against 68b51f5

@facebook-github-botfacebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Sep 27, 2022
Comment threadyarn.lock Outdated
Comment on lines 6309 to 6202
metro-react-native-babel-transformer@^0.72.1:
version "0.72.1"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Unfortunately I'm not sure this solves the "multiple versions" problem, because you get issues like this where Yarn won't update if it has an existing entry that satisfies the semver range for a given dependency - there are a mix of 0.72.3 and 0.72.1 in this resulting lockfile - and app developers will get the same issue in their project lockfiles.

I think the only way to ensure all the Metro packages to reliably stay aligned is for everything that depends on them to pin them exactly, but I'm open to suggestions..

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

ok - I just did what @thymikee recommended to me as bumps; I always tend do prefer fixed versions. I assumed that was an agreement between CLI and Metro?

Mike, what should we do?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think you may be right @robhogan, I recall situations like this. Although I never really bothered as an app dev, because I edited the lockfile or ran yarn-deduplicate. As a library author I prefer using ^ or ~ instead of fix versions, so that transitive deps can "fix themselves" without me issuing a new version. But, it looks like in RN repo Metro is always fixed to a specific version. I'm good with doing the same for the CLI, if we agree that's the way it should be.

If no-one is opposed to this, I'll issue CLI 9.1.3 with Metro fixed to 0.72.3 (or whatever is the latest) tomorrow. Then this PR could be updated to 0.72.3 as well, to get the latest fixes :)

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

ok, then let's wait for 9.1.3 and then do all fixed versions

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

(I might've missed some discussion here so apologies if I'm contradicting some other conclusion, but yeah I think it's clear from the lockfile that this won't necessarily help - Yarn is conservative by design and won't necessarily update everything in step when a lockfile is present.)

A big part of why this multiple versions thing is actually a problem is because Metro uses dynamic require calls to provide configurable/pluggable bits of functionality, eg const transformer = require(babelTransformerPath). Those "dependencies" aren't necessarily explicit in the package.json of the requiring package, and unless they're absolute paths there's no real telling what they'll resolve to, which is how we get invocations crossing version boundaries within Metro even though all of Metro's packages use exact pinned dependencies. I think solving that would mean moving from configuration by dynamic require, to configuration by code injection, and (strictly IMO) we should fix all of that in Metro.

In the mean time, I don't see an alternative other than exact pinning everywhere and making sure they're kept consistent.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@robhogan let's handle that separately - I want to be able to cherry pick this into the 0.70 release

@motiz88motiz88Sep 28, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For context, there was a brief discussion of exact vs caret dependencies in #releases-coordination on Discord, where @thymikee and myself were in favour of caret dependencies: https://discord.com/channels/514829729862516747/514832743654228009/1022065446305923073

Based on @robhogan's comments it does seem that exact deps on Metro would be more reliable for our users. But... doesn't it mean we would also need the RN-->CLI dependency to be exact? (Otherwise the CLI can't safely upgrade its pinned version of Metro in a patch/minor version)

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

RN-->CLI dependency to be exact? (Otherwise the CLI can't safely upgrade its pinned version of Metro in a patch version)

yeah I assume that's right, once the new CLI release with the pinned version of Metro is out I'll update this to pin everything; and then we can keep this pattern going for the Metro 73 updates (which will likely happen around RN71)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Released v9.1.3

@kelset
kelsetforce-pushed the kelset/bump-metro-and-cli branch from 9c4d0c2 to 18de172CompareSeptember 30, 2022 09:35
@kelset

Copy link
Copy Markdown
ContributorAuthor

@robhogan@thymikee@motiz88 updated the PR with latest and all pinned. LMK if now it's good to go

@kelsetkelset changed the title chore(deps): bump CLI to 9.1.2 and Metro to ^0.72.1chore(deps): bump CLI to 9.1.3 and Metro to 0.72.3Sep 30, 2022
@analysis-bot

Copy link
Copy Markdown
PlatformEngineArchSize (bytes)Diff
androidhermesarm64-v8a7,736,904+221
androidhermesarmeabi-v7a7,141,724+217
androidhermesx868,046,297+223
androidhermesx86_648,018,521+222
androidjscarm64-v8a9,605,443+51
androidjscarmeabi-v7a8,373,044+54
androidjscx869,551,383+57
androidjscx86_6410,144,396+56

Base commit: 3037190
Branch: main

@analysis-bot

Copy link
Copy Markdown
PlatformEngineArchSize (bytes)Diff
ios-universaln/a--

Base commit: 3037190
Branch: main

@robhoganrobhogan left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM, thanks @kelset!

@facebook-github-bot

Copy link
Copy Markdown
Contributor

@robhogan has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@react-native-bot

Copy link
Copy Markdown
Collaborator

This pull request was successfully merged by @kelset in f164556.

When will my fix make it into a release? | Upcoming Releases

@react-native-botreact-native-bot added the Merged This PR has been merged. label Sep 30, 2022
@kelset
kelset deleted the kelset/bump-metro-and-cli branch September 30, 2022 13:48
kelset added a commit that referenced this pull request Oct 3, 2022
Summary:
This PR bumps the dep version of Metro and the RN CLI to latest, and realigns them to avoid the issue we currently have in 0.70: #34714 (this commit will be cherry-picked there)
Also, it pins it all down to precise version. See comments for reasoning.
While at it, I gave a cleanup pass to the yarn.lock with [`yarn deduplicate`](https://github.com/scinos/yarn-deduplicate#readme).
## Changelog
<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->
[General] [Changed] - bump CLI to 9.1.3 and Metro to 0.72.3
Pull Request resolved: #34803
Test Plan: CI is green
Reviewed By: dmytrorykun
Differential Revision: D39967392
Pulled By: dmytrorykun
fbshipit-source-id: 799dd745834c9ba349362f70afb4bdbd1a48260e
# Conflicts:
#	package.json
#	template/package.json
#	yarn.lock
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.ContributorA React Native contributor.MergedThis PR has been merged.p: MicrosoftPartner: MicrosoftPartnerShared with MetaApplied via automation to indicate that an Issue or Pull Request has been shared with the team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@kelset@analysis-bot@facebook-github-bot@react-native-bot@motiz88@robhogan@thymikee@dmytrorykun