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

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

Closed
wants to merge 3 commits into from

Conversation

kelset
Copy link
Collaborator

@kelset kelset commented Sep 27, 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.

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 owner Sep 27, 2022
@facebook-github-bot facebook-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
@kelset kelset requested review from cortinico and dmytrorykun Sep 27, 2022
@github-actions
Copy link

github-actions bot commented Sep 27, 2022

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-bot facebook-github-bot added the Shared with React Native Team Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Sep 27, 2022
yarn.lock Outdated
metro-react-native-babel-transformer@^0.72.1:
version "0.72.1"
Copy link
Contributor

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
Collaborator Author

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
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
Collaborator Author

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
Contributor

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
Collaborator Author

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

Copy link
Contributor

@motiz88 motiz88 Sep 28, 2022

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
Collaborator Author

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
Contributor

Choose a reason for hiding this comment

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

Copy link
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 kelset force-pushed the kelset/bump-metro-and-cli branch from 9c4d0c2 to 18de172 Compare Sep 30, 2022
@kelset
Copy link
Collaborator Author

kelset commented Sep 30, 2022

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

@kelset kelset requested a review from robhogan Sep 30, 2022
@kelset kelset changed the title chore(deps): bump CLI to 9.1.2 and Metro to ^0.72.1 chore(deps): bump CLI to 9.1.3 and Metro to 0.72.3 Sep 30, 2022
@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,736,904 +221
android hermes armeabi-v7a 7,141,724 +217
android hermes x86 8,046,297 +223
android hermes x86_64 8,018,521 +222
android jsc arm64-v8a 9,605,443 +51
android jsc armeabi-v7a 8,373,044 +54
android jsc x86 9,551,383 +57
android jsc x86_64 10,144,396 +56

Base commit: 3037190
Branch: main

@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: 3037190
Branch: main

Copy link
Contributor

@robhogan robhogan left a comment

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
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
Collaborator

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

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

@react-native-bot react-native-bot added the Merged This PR has been merged. label Sep 30, 2022
@kelset kelset deleted the kelset/bump-metro-and-cli branch Sep 30, 2022
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 free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. Merged This PR has been merged. p: Microsoft Partner: Microsoft Partner Shared with React Native Team Applied 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.

None yet

8 participants