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

Make sure js bundle still exists at bundle-output path #30149

Closed
wants to merge 1 commit into from

Conversation

janicduplessis
Copy link
Contributor

Summary

Since changes to support hermes on iOS the js bundled is moved away from the location where it is generated when calling metro. This causes issues with the RN sentry integration since it relies on intercepting this path to find the bundle file after running react-native-xcode.sh. Seems kind of like a hacky way to get the bundle location, but let's avoid breaking it.

https://github.com/getsentry/sentry-cli/blob/master/src/commands/react_native_xcode.rs

Changelog

[iOS] [Fixed] - Make sure js bundle still exists at bundle-output path

Test Plan

Checked that the bundle file exists both at bundle-output path and in the .app.
Checked that the sentry release script works.

@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. labels Oct 9, 2020
@react-native-bot react-native-bot added Bug Platform: iOS iOS applications. labels Oct 9, 2020
@janicduplessis
Copy link
Contributor Author

cc @alloy

@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,385,801 0
android hermes armeabi-v7a 7,013,117 0
android hermes x86 7,828,013 0
android hermes x86_64 7,717,499 0
android jsc arm64-v8a 9,532,102 0
android jsc armeabi-v7a 9,147,130 -4,096
android jsc x86 9,396,778 0
android jsc x86_64 9,978,488 0

Base commit: 0d02c60

@analysis-bot
Copy link

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

Base commit: 0d02c60

Copy link
Contributor

@alloy alloy left a comment

Choose a reason for hiding this comment

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

Seems fair enough to me to not break that, even if not being a hard guarantee 👍

/cc @appden

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @janicduplessis in 3a41f69.

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 Oct 23, 2020
@janicduplessis janicduplessis deleted the rn-xcode-out branch October 23, 2020 01:20
lmarques6 pushed a commit to TeamGuilded/react-native that referenced this pull request Oct 27, 2020
Summary:
Since changes to support hermes on iOS the js bundled is moved away from the location where it is generated when calling metro. This causes issues with the RN sentry integration since it relies on intercepting this path to find the bundle file after running react-native-xcode.sh. Seems kind of like a hacky way to get the bundle location, but let's avoid breaking it.

https://github.com/getsentry/sentry-cli/blob/master/src/commands/react_native_xcode.rs

## Changelog

[iOS] [Fixed] - Make sure js bundle still exists at bundle-output path

Pull Request resolved: facebook#30149

Test Plan:
Checked that the bundle file exists both at bundle-output path and in the .app.
Checked that the sentry release script works.

Reviewed By: cpojer

Differential Revision: D24480115

Pulled By: appden

fbshipit-source-id: c01c80d47ed54319f97063ec635c021552a95c22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug 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. Platform: iOS iOS applications.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants