Remove "bytecodeFileName" concept from "microsoft/react-native" #183

NickGerleman · 2019-10-31T01:15:21Z

Summary

bytecodeFileName is used excusively by ChakraExecutor in react-native-windows, and diverges us from upstream. Remove the concept in favor of passing bytecodeFileName directly to the ChakraJsExecutorFactory at creation time. We currently use a single bytecode file per-instance , so we see no loss of functionality when moving to a 1:1 relationship between executor and bytecode file.

Locally validated that Android NDK libs still build. Verified we can build react-native-windows after some small changes that must be commited separately.

Please select one of the following

I am removing an existing difference between facebook/react-native and microsoft/react-native 👍
I am cherry-picking a change from Facebook's react-native into microsoft/react-native 👍
I am making a fix / change for the macOS implementation of react-native
I am making a change required for Microsoft usage of react-native

Microsoft Reviewers: Open in CodeFlow

msftclas · 2019-10-31T01:15:34Z

All CLA requirements met.

pull-bot · 2019-10-31T01:17:06Z

	Messages
📖	📋 Missing Changelog - Can you add a Changelog? To do so, add a "## Changelog" section to your PR description. A changelog entry has the following format: `[CATEGORY] [TYPE] - Message`. CATEGORY may be: General iOS Android JavaScript Internal (for changes that do not need to be called out in the release notes) TYPE may be: Added, for new features. Changed, for changes in existing functionality. Deprecated, for soon-to-be removed features. Removed, for now removed features. Fixed, for any bug fixes. Security, in case of vulnerabilities. MESSAGE may answer "what and why" on a feature level. Use this to briefly tell React Native users about notable changes.
📖	📋 Missing Test Plan - Can you add a Test Plan? To do so, add a "## Test Plan" section to your PR description. A Test Plan lets us know how these changes were tested.
📖	📋 Missing Summary - Can you add a Summary? To do so, add a "## Summary" section to your PR description. This is a good place to explain the motivation for making this change.

Generated by 🚫 dangerJS against e175841

bytecodeFileName is used excusively by ChakraExecutor in react-native-windows, and diverges us from upstream. Remove the concept in favor of passing bytecodeFileName directly to the ChakraJsExecutorFactory at creation time. We currently use a single bytecode file per-instance , so we see no loss of functionality when moving to a 1:1 relationship between executor and bytecode file. Locally validated that Android NDK libs still build. Verified we can build react-native-windows after some small changes that must be commited separately.

NickGerleman · 2019-10-31T03:36:31Z

@hansenyy is added to the review. #Closed

NickGerleman · 2019-10-31T03:38:27Z

@vmoroz is added to the review. #Closed

NickGerleman · 2019-10-31T03:48:35Z

React/CxxBridge/RCTCxxBridge.mm

    } else if (reactInstance) {
-      reactInstance->loadScriptFromString(std::make_unique<NSDataBigString>(script), 0,
-                                                 sourceUrlStr.UTF8String, !async, ""); // TODO(OSS Candidate ISS#2710739)
+      reactInstance->loadScriptFromString(std::make_unique<NSDataBigString>(script), 0, // TODO(OSS Candidate ISS#2710739)


the 0 here is for bundleVersion which still differs

acoates-ms · 2019-10-31T16:29:12Z

Are you sure about this?: "We currently use a single bytecode file per-instance , so we see no loss of functionality when moving to a 1:1 relationship between executor and bytecode file."

Don't we use platform bundles in devmain? I mean, I think this should still be done through calls directly to the executor, just checking that you have all the downstream changes thought through to devmain that would be required to land this change.

vmoroz · 2019-10-31T17:36:14Z

This change will require a lot of downstream changes. Could you prepare them to be ready right after you submit this change?

NickGerleman · 2019-10-31T18:15:18Z

@vmoroz @acoates-ms I have a separate change ready to go for react-native-windows, and I don't believe we should cause breakage in devmain, though I may be missing something. I can send out a separate PR for that if interested, though it cannot be merged until we publish a new npm package from here.

On the react-native-windows side, InstanceImpl seems to exclusively feed the bytecode filename from DevSettings to the ChakraExecutor. InstanceWrapper doesn't seem to expose a mechanism to set bytecodeFilename apart from DevSettings, so it seems like there's no interface changes unless something in devmain is using something at a lower level of abstraction. @hansenyy could you confirm?

Related. I noticed something weird in CWinReactImpl::InitializeThis in devmain, where we have some logic to set bytecode filename in devSettings but never actually seem to use this. Is this just NYI, or should we have a bug on this? See "/reactnativehost/ReactImpl_Win.cpp:L190" (Guessing we can't put devmain code in public comments?)

We currently determine bytecodeFileName when creating InstanceImpl, determined by the passed in DevSettings. This change makes it so that we pass the bytecodeFilename as an InstanceArg to the ChakraExecutorFactory instead of passing it through public JSInstance and JSExecutor APIs. This change is dependent on microsoft/react-native-macos#183 in react-native. Haven't tested this end-to-end yet (having issues doing it locally). Will get that sorted out before merging either PR, since we cannot rely on CI until we check in the RN change.

vmoroz · 2019-10-31T18:52:49Z

The devmain code you are referring to is obsolete. Instead, we call the loadApplication explicitly. Let's not discuss it in this public repo. I would expect that we change the JSExecutor to map the JS bundle to bytecode. I guess it is part of react-native-windows.

In reply to: 548503903 [](ancestors = 548503903)

vmoroz

We currently determine bytecodeFileName when creating InstanceImpl, determined by the passed in DevSettings. This change makes it so that we pass the bytecodeFilename as an InstanceArg to the ChakraExecutorFactory instead of passing it through public JSInstance and JSExecutor APIs. This change is dependent on microsoft/react-native-macos#183 in react-native. Haven't tested this end-to-end yet (having issues doing it locally). Will get that sorted out before merging either PR, since we cannot rely on CI until we check in the RN change.

hansenyy

NickGerleman · 2019-11-01T02:26:55Z

Going to abandon this as is and revisit with something a bit more comprehensive. There are the platform bundle concerns that were brought up which necessitate some further changes, but I don't love the story we leave here for bytecode logic in general, where we have versioning left in ms/rn but path logic in devmain. This is further complicated by JSI executors moving to PreparedScriptStore, meaning we need to be intentional in making whatever we introduce easy to rip out.

We currently determine bytecodeFileName when creating InstanceImpl, determined by the passed in DevSettings. This change makes it so that we pass the bytecodeFilename as an InstanceArg to the ChakraExecutorFactory instead of passing it through public JSInstance and JSExecutor APIs. This change is dependent on microsoft/react-native-macos#183 in react-native. Haven't tested this end-to-end yet (having issues doing it locally). Will get that sorted out before merging either PR, since we cannot rely on CI until we check in the RN change.

…#3591) * Update to react-native@0.60.0-microsoft.16 * Change files * Change files * Move bytecodeFileName setting to ChakraExecutorFactory We currently determine bytecodeFileName when creating InstanceImpl, determined by the passed in DevSettings. This change makes it so that we pass the bytecodeFilename as an InstanceArg to the ChakraExecutorFactory instead of passing it through public JSInstance and JSExecutor APIs. This change is dependent on microsoft/react-native-macos#183 in react-native. Haven't tested this end-to-end yet (having issues doing it locally). Will get that sorted out before merging either PR, since we cannot rely on CI until we check in the RN change. * Move from a static bytecodeFilename in DevSettings to a newly introduced ScriptByteCodeResolver, letting us inject some logic in how to determine bytecode from URIs. * Add ScriptMetadataMap for ChakraExecutor We're going to move to a new design for bytecode caching/prepared scripts when moving to JSI. In the meantime, we can reuse most of the existing ChakraExecutor bytecode and script versioning logic. Pass this association in as a map to DevSettings instead of piping it through each call. * Remove some more dead code * Use member DevSettings in InstanceImpl instead of dead parameter * Rename some things. Change the metadata map to be stored by value instead of pointer to avoid dual null states of empty map and actually null. This should be fine from a perf perspective since we don't have many bundles, and we avoid an extra heap allocation for the shared_ptr. * Update lockfile * Yarn format * UWP Build Fixes

Summary: Pull Request resolved: facebook#52317 Changelog: [Internal] - Update `react-native/debugger-frontend` from d95ac13...35c4630 Resyncs `react-native/debugger-frontend` from GitHub - see `rn-chrome-devtools-frontend` [changelog](facebook/react-native-devtools-frontend@d95ac13...35c4630). ### Changelog | Commit | Author | Date/Time | Subject | | ------ | ------ | --------- | ------- | | [35c4630bd](facebook/react-native-devtools-frontend@35c4630bd) | Vitali Zaidman (vzaidman@gmail.com) | 2025-06-26T09:47:45+01:00 | [track stack trace symbolication failures (microsoft#183)](facebook/react-native-devtools-frontend@35c4630bd) | | [e4487ef2e](facebook/react-native-devtools-frontend@e4487ef2e) | Vitali Zaidman (vzaidman@gmail.com) | 2025-06-24T14:29:35+01:00 | [support symbolication with native frames (microsoft#181)](facebook/react-native-devtools-frontend@e4487ef2e) | Reviewed By: huntie Differential Revision: D77437936 fbshipit-source-id: 5b64195064aa5e157ae1adad01a3f2dc94045f98

NickGerleman force-pushed the bc-filename branch from ebf1f17 to 35f4389 Compare October 31, 2019 03:11

NickGerleman force-pushed the bc-filename branch from 35f4389 to e175841 Compare October 31, 2019 03:12

NickGerleman marked this pull request as ready for review October 31, 2019 03:29

NickGerleman requested a review from acoates-ms as a code owner October 31, 2019 03:29

NickGerleman commented Oct 31, 2019

View reviewed changes

vmoroz approved these changes Oct 31, 2019

View reviewed changes

NickGerleman mentioned this pull request Oct 31, 2019

Move bytecodeFileName setting to ChakraExecutorFactory microsoft/react-native-windows#3571

Closed

hansenyy approved these changes Oct 31, 2019

View reviewed changes

NickGerleman closed this Nov 1, 2019

NickGerleman deleted the bc-filename branch November 6, 2019 09:51

Remove "bytecodeFileName" concept from "microsoft/react-native" #183

Remove "bytecodeFileName" concept from "microsoft/react-native" #183

Uh oh!

Conversation

NickGerleman commented Oct 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Please select one of the following

Microsoft Reviewers: Open in CodeFlow

Uh oh!

msftclas commented Oct 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pull-bot commented Oct 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NickGerleman commented Oct 31, 2019

Uh oh!

NickGerleman commented Oct 31, 2019

Uh oh!

NickGerleman Oct 31, 2019

Choose a reason for hiding this comment

Uh oh!

acoates-ms commented Oct 31, 2019

Uh oh!

vmoroz commented Oct 31, 2019

Uh oh!

NickGerleman commented Oct 31, 2019

Uh oh!

vmoroz commented Oct 31, 2019

Uh oh!

vmoroz left a comment

Choose a reason for hiding this comment

Uh oh!

hansenyy left a comment

Choose a reason for hiding this comment

Uh oh!

NickGerleman commented Nov 1, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

NickGerleman commented Oct 31, 2019 •

edited

Loading

msftclas commented Oct 31, 2019 •

edited

Loading

pull-bot commented Oct 31, 2019 •

edited

Loading