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

feat: prevent removing globalEvalWithSourceUrl in RELEASE builds #34319

Conversation

EvanBacon
Copy link
Contributor

Summary

Expo Go was using nativeInjectHMRUpdate for snack to provide stack traces to OTA errors. nativeInjectHMRUpdate was replaced with globalEvalWithSourceUrl here. The issue with globalEvalWithSourceUrl is that it is stripped in RELEASE builds, and Expo Go is installed via the App Store, making it a release build.

I propose we keep the method, eval is still exposed in production so there's no increase in security risk, simply provides a better DX for multipurpose development clients.

Changelog

[Android] [Added] - Expose globalEvalWithSourceUrl in production builds.
[iOS] [Added] - Expose globalEvalWithSourceUrl in production builds.

Test Plan

  • Function should be available as global.globalEvalWithSourceUrl in a production build.

@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. p: Expo Partner: Expo Partner labels Aug 1, 2022
@react-native-bot react-native-bot added Platform: Android Android applications. Type: Enhancement A new feature or enhancement of an existing feature. labels Aug 1, 2022
@facebook-github-bot facebook-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 Aug 1, 2022
@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,834,767 +833
android hermes armeabi-v7a 7,225,447 +2,221
android hermes x86 8,146,080 +940
android hermes x86_64 8,125,785 +1,169
android jsc arm64-v8a 9,710,701 +1,014
android jsc armeabi-v7a 8,463,650 +853
android jsc x86 9,659,848 +1,016
android jsc x86_64 10,259,134 +862

Base commit: 1e4ebf2
Branch: main

@analysis-bot
Copy link

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

Base commit: 9797388
Branch: main

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @EvanBacon in 333583b.

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 Aug 4, 2022
@EvanBacon EvanBacon deleted the @evanbacon/allow-globalEvalWithSourceUrl-in-release branch August 12, 2022 15:23
roryabraham pushed a commit to Expensify/react-native that referenced this pull request Aug 17, 2022
…ebook#34319)

Summary:
Expo Go was using `nativeInjectHMRUpdate` for [snack](https://snack.expo.io/) to provide stack traces to OTA errors. `nativeInjectHMRUpdate` was replaced with `globalEvalWithSourceUrl` [here](facebook@71c84cf). The issue with `globalEvalWithSourceUrl` is that it is stripped in RELEASE builds, and Expo Go is installed via the App Store, making it a release build.

I propose we keep the method, `eval` is still exposed in production so there's no increase in security risk, simply provides a better DX for multipurpose development clients.

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

[Android] [Added] - Expose `globalEvalWithSourceUrl` in production builds.
[iOS] [Added] - Expose `globalEvalWithSourceUrl` in production builds.

Pull Request resolved: facebook#34319

Test Plan: - Function should be available as `global.globalEvalWithSourceUrl` in a production build.

Reviewed By: christophpurrer

Differential Revision: D38312111

Pulled By: motiz88

fbshipit-source-id: adcd83ad1103c8fd4c6d5d7aec765f27881e0432
roryabraham pushed a commit to Expensify/react-native that referenced this pull request Aug 17, 2022
…ebook#34319)

Summary:
Expo Go was using `nativeInjectHMRUpdate` for [snack](https://snack.expo.io/) to provide stack traces to OTA errors. `nativeInjectHMRUpdate` was replaced with `globalEvalWithSourceUrl` [here](facebook@71c84cf). The issue with `globalEvalWithSourceUrl` is that it is stripped in RELEASE builds, and Expo Go is installed via the App Store, making it a release build.

I propose we keep the method, `eval` is still exposed in production so there's no increase in security risk, simply provides a better DX for multipurpose development clients.

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

[Android] [Added] - Expose `globalEvalWithSourceUrl` in production builds.
[iOS] [Added] - Expose `globalEvalWithSourceUrl` in production builds.

Pull Request resolved: facebook#34319

Test Plan: - Function should be available as `global.globalEvalWithSourceUrl` in a production build.

Reviewed By: christophpurrer

Differential Revision: D38312111

Pulled By: motiz88

fbshipit-source-id: adcd83ad1103c8fd4c6d5d7aec765f27881e0432
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. Merged This PR has been merged. p: Expo Partner: Expo Partner Platform: Android Android applications. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. Type: Enhancement A new feature or enhancement of an existing feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants