Remove scriptVersion/bundleVersion and bytecodeFileName Parameters #187

NickGerleman · 2019-11-04T23:24:08Z

See accompanying changes in react-native-windows in microsoft/react-native-windows#3591

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

pull-bot · 2019-11-04T23:27:34Z

	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 de0c9e0

NickGerleman · 2019-11-05T06:22:23Z

Will revert this #Resolved

Refers to: .gitignore:100 in 362847f. [](commit_id = 362847f, deletion_comment = False)

vmoroz · 2019-11-05T22:15:37Z

ReactCommon/cxxreact/Instance.cpp

-                               uint64_t bundleVersion, // TODO(OSS Candidate ISS#2710739)
-                               std::string bundleURL,
-                               std::string&& bytecodeFileName) { // TODO(OSS Candidate ISS#2710739)
+                               std::string bundleURL) { // TODO(OSS Candidate ISS#2710739)


// TODO(OSS Candidate ISS#2710739) [](start = 56, length = 34)

Remove? #ByDesign

We do have a small change in naming, where upstream calls this sourcrUrl. I've seen some of the naming changes marked, so I think it makes sense to keep the comment since we still differ from upstream (admittedly not very much).

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

vmoroz

NikoAri · 2019-11-05T22:28:21Z

ReactCommon/cxxreact/Instance.h

-                            std::string&& bytecodeFileName);
+  virtual void loadScriptFromString(
+      std::unique_ptr<const JSBigString> bundleString,
+      std::string bundleURL,


string bundleURL [](start = 11, length = 16)

why not pass it as a & or &&? #ByDesign

I agree, but I'm not wanting to change existing interfaces for upstream RN.

There is a school of thought that you should accept move-only types by value instead of r-value, but I don't think that makes a lot of sense for something like string. They might just do a move internally, in which case that's no worse than taking in a const-ref and doing a copy.

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

NikoAri · 2019-11-05T22:28:35Z

ReactCommon/cxxreact/Instance.h

                        std::shared_ptr<MessageQueueThread> jsQueue,
                        std::shared_ptr<ModuleRegistry> moduleRegistry);

  void setSourceURL(std::string sourceURL);


ng [](start = 29, length = 2)

this should be && too #ByDesign

Not wanting to change existing interfaces for upstream RN. See other comment.

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

NikoAri · 2019-11-05T22:29:11Z

ReactCommon/cxxreact/Instance.h

-                           uint64_t bundleVersion, // TODO(OSS Candidate ISS#2710739)
-                           std::string bundleURL,
-                           std::string&& bytecodeFileName); // TODO(OSS Candidate ISS#2710739)
+                           std::string bundleURL);


g [](start = 37, length = 1)

&&? #ByDesign

Not wanting to change existing interfaces for upstream RN. See other comment.

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

NikoAri

hansenyy · 2019-11-06T00:16:18Z

#ifndef V8_V8EXECUTOR_H

From what I understand V8Executor is no longer being used. We should remove them. #Resolved

Refers to: ReactCommon/cxxreact/V8Executor.h:1 in de0c9e0. [](commit_id = de0c9e0, deletion_comment = True)

hansenyy

NickGerleman · 2019-11-06T00:39:06Z

#ifndef V8_V8EXECUTOR_H

I know it at least compiles on Android, but not sure anything more. WIll log an item.

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

Refers to: ReactCommon/cxxreact/V8Executor.h:1 in 362847f. [](commit_id = 362847f, deletion_comment = True)

bytecodeFileName is used excusively by ChakraExecutor in react-native-windows, and diverges us from upstream. Remove these parameters, using a new mechanism in react-native-windows.

Summary: Pull Request resolved: facebook#52486 Changelog: [Internal] - Update `react-native/debugger-frontend` from 51a91a2...844f225 Resyncs `react-native/debugger-frontend` from GitHub - see `rn-chrome-devtools-frontend` [changelog](facebook/react-native-devtools-frontend@51a91a2...844f225). ### Changelog | Commit | Author | Date/Time | Subject | | ------ | ------ | --------- | ------- | | [844f22500](facebook/react-native-devtools-frontend@844f22500) | Vitali Zaidman (vzaidman@gmail.com) | 2025-07-08T10:16:09+01:00 | [Track when the stack trace symbolication succeeds (microsoft#187)](facebook/react-native-devtools-frontend@844f22500) | | [ffa6bb6af](facebook/react-native-devtools-frontend@ffa6bb6af) | Vitali Zaidman (vzaidman@gmail.com) | 2025-07-07T16:21:55+01:00 | [parse "empty url" and "address at" frames as non-hyperlinked text (microsoft#188)](facebook/react-native-devtools-frontend@ffa6bb6af) | Reviewed By: robhogan Differential Revision: D77926362 fbshipit-source-id: cbcf8b17cd175400d3d027ce2e8ebefa497f5d79

NickGerleman requested review from a team, NikoAri and hansenyy November 5, 2019 04:27

NickGerleman marked this pull request as ready for review November 5, 2019 04:34

NickGerleman requested a review from acoates-ms as a code owner November 5, 2019 04:34

vmoroz reviewed Nov 5, 2019

View reviewed changes

vmoroz approved these changes Nov 5, 2019

View reviewed changes

NikoAri reviewed Nov 5, 2019

View reviewed changes

NikoAri approved these changes Nov 5, 2019

View reviewed changes

hansenyy approved these changes Nov 6, 2019

View reviewed changes

Remove bytecodeFileName and scriptVersion/bundleVersion parameters

de0c9e0

bytecodeFileName is used excusively by ChakraExecutor in react-native-windows, and diverges us from upstream. Remove these parameters, using a new mechanism in react-native-windows.

NickGerleman force-pushed the bundleVersion branch from 362847f to de0c9e0 Compare November 6, 2019 03:44

NickGerleman merged commit 9bba27f into microsoft:master Nov 6, 2019

NickGerleman deleted the bundleVersion branch November 6, 2019 09:51

Remove scriptVersion/bundleVersion and bytecodeFileName Parameters #187

Remove scriptVersion/bundleVersion and bytecodeFileName Parameters #187

Uh oh!

Conversation

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

Uh oh!

Please select one of the following

Microsoft Reviewers: Open in CodeFlow

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

vmoroz Nov 5, 2019 • edited by NickGerleman Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NickGerleman Nov 5, 2019

Choose a reason for hiding this comment

Uh oh!

vmoroz left a comment

Choose a reason for hiding this comment

Uh oh!

NikoAri Nov 5, 2019 • edited by NickGerleman Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NickGerleman Nov 5, 2019

Choose a reason for hiding this comment

Uh oh!

NikoAri Nov 5, 2019 • edited by NickGerleman Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NickGerleman Nov 5, 2019

Choose a reason for hiding this comment

Uh oh!

NikoAri Nov 5, 2019 • edited by NickGerleman Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NickGerleman Nov 5, 2019

Choose a reason for hiding this comment

Uh oh!

NikoAri left a comment

Choose a reason for hiding this comment

Uh oh!

hansenyy commented Nov 6, 2019 • edited by NickGerleman Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hansenyy left a comment

Choose a reason for hiding this comment

Uh oh!

NickGerleman commented Nov 6, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

NickGerleman commented Nov 4, 2019 •

edited

Loading

pull-bot commented Nov 4, 2019 •

edited

Loading

NickGerleman commented Nov 5, 2019 •

edited

Loading

vmoroz Nov 5, 2019 •

edited by NickGerleman

Loading

NikoAri Nov 5, 2019 •

edited by NickGerleman

Loading

NikoAri Nov 5, 2019 •

edited by NickGerleman

Loading

NikoAri Nov 5, 2019 •

edited by NickGerleman

Loading

hansenyy commented Nov 6, 2019 •

edited by NickGerleman

Loading