Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/chatty-states-build.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"react-native-node-api": patch
---

Add --react-native-package option to "vendor-hermes" command, allowing caller to choose the package to download hermes into
23 changes: 15 additions & 8 deletions packages/host/scripts/patch-hermes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,20 @@
raise "React Native Node-API cannot reliably patch JSI when React Native Core is prebuilt."
end

VENDORED_HERMES_DIR ||= `npx react-native-node-api vendor-hermes --silent '#{Pod::Config.instance.installation_root}'`.strip
if Dir.exist?(VENDORED_HERMES_DIR)
Pod::UI.info "Hermes vendored into #{VENDORED_HERMES_DIR.inspect}"
else
raise "Hermes patching failed. Please check the output above for errors."
if ENV['REACT_NATIVE_OVERRIDE_HERMES_DIR'].nil?
VENDORED_HERMES_DIR ||= `npx react-native-node-api vendor-hermes --silent '#{Pod::Config.instance.installation_root}'`.strip
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there any reason for this to be ||= rather than =? As VENDORED_HERMES_DIR is a local variable, I think it can only be unset at this point?

Suggested change
VENDORED_HERMES_DIR ||= `npx react-native-node-api vendor-hermes --silent '#{Pod::Config.instance.installation_root}'`.strip
VENDORED_HERMES_DIR = `npx react-native-node-api vendor-hermes --silent '#{Pod::Config.instance.installation_root}'`.strip

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's my understanding that Ruby scripts aren't caching requires the same way Node.js would.

I had this originally because it prevents re-execution of the command if the .rb file is (transitively) "required" multiple times.

Comment on lines +7 to +8
Copy link
Collaborator

Choose a reason for hiding this comment

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

Summary of the proposed behaviour for my benefit:

  • If ENV.REACT_NATIVE_OVERRIDE_HERMES_DIR is defined:
    • The env var is used as-is.
    • The script checks whether the directory is exists.
      • If it exists, then good to go.
      • If not, an exception is raised.
  • If ENV.REACT_NATIVE_OVERRIDE_HERMES_DIR is not defined
    • npx react-native-node-api vendor-hermes --silent is called, ultimately vendoring Hermes for react-native (regardless of whether the user is using react-native or react-native-macos.
    • That vendored directory gets set as the value for ENV.REACT_NATIVE_OVERRIDE_HERMES_DIR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Exactly and I agree it would be ideal if we didn't vendor for react-native if this was driven by a MacOS app. Let's make that happen in a follow-up PR, ideally when we actually have a MacOS app in the repo, testing that flow.

# Signal the patched Hermes to React Native
ENV['BUILD_FROM_SOURCE'] = 'true'
ENV['REACT_NATIVE_OVERRIDE_HERMES_DIR'] = VENDORED_HERMES_DIR
elsif Dir.exist?(ENV['REACT_NATIVE_OVERRIDE_HERMES_DIR'])
# Setting an override path implies building from source
ENV['BUILD_FROM_SOURCE'] = 'true'
end

# Signal the patched Hermes to React Native
ENV['BUILD_FROM_SOURCE'] = 'true'
ENV['REACT_NATIVE_OVERRIDE_HERMES_DIR'] = VENDORED_HERMES_DIR
if !ENV['REACT_NATIVE_OVERRIDE_HERMES_DIR'].empty?
if Dir.exist?(ENV['REACT_NATIVE_OVERRIDE_HERMES_DIR'])
Pod::UI.info "[Node-API] Using overridden Hermes in #{ENV['REACT_NATIVE_OVERRIDE_HERMES_DIR'].inspect}"
else
raise "Hermes patching failed: Expected override to exist in #{ENV['REACT_NATIVE_OVERRIDE_HERMES_DIR'].inspect}"
end
end
31 changes: 25 additions & 6 deletions packages/host/src/node/cli/hermes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,18 +5,24 @@ import path from "node:path";
import {
chalk,
Command,
Option,
oraPromise,
spawn,
UsageError,
wrapAction,
prettyPath,
} from "@react-native-node-api/cli-utils";
import { packageDirectorySync } from "pkg-dir";
import { packageDirectory } from "pkg-dir";
import { readPackage } from "read-pkg";

const HOST_PACKAGE_ROOT = path.resolve(__dirname, "../../..");
// FIXME: make this configurable with reasonable fallback before public release
const HERMES_GIT_URL = "https://github.com/kraenhansen/hermes.git";

const platformOption = new Option(
"--react-native-package <package-name>",
"The React Native package to vendor Hermes into",
).default("react-native");

export const command = new Command("vendor-hermes")
.argument("[from]", "Path to a file inside the app package", process.cwd())
.option("--silent", "Don't print anything except the final path", false)
Expand All @@ -25,12 +31,20 @@ export const command = new Command("vendor-hermes")
"Don't check timestamps of input files to skip unnecessary rebuilds",
false,
)
.addOption(platformOption)
.action(
wrapAction(async (from, { force, silent }) => {
const appPackageRoot = packageDirectorySync({ cwd: from });
wrapAction(async (from, { force, silent, reactNativePackage }) => {
const appPackageRoot = await packageDirectory({ cwd: from });
assert(appPackageRoot, "Failed to find package root");

const { dependencies = {} } = await readPackage({ cwd: appPackageRoot });
assert(
Object.keys(dependencies).includes(reactNativePackage),
`Expected app to have a dependency on the '${reactNativePackage}' package`,
);

const reactNativePath = path.dirname(
require.resolve("react-native/package.json", {
require.resolve(reactNativePackage + "/package.json", {
// Ensures we'll be patching the React Native package actually used by the app
paths: [appPackageRoot],
}),
Expand All @@ -40,6 +54,11 @@ export const command = new Command("vendor-hermes")
"sdks",
".hermesversion",
);
assert(
fs.existsSync(hermesVersionPath),
`Expected a file with a Hermes version at ${prettyPath(hermesVersionPath)}`,
);

const hermesVersion = fs.readFileSync(hermesVersionPath, "utf8").trim();
if (!silent) {
console.log(`Using Hermes version: ${hermesVersion}`);
Expand All @@ -50,7 +69,7 @@ export const command = new Command("vendor-hermes")
"ReactCommon/jsi/jsi/",
);

const hermesPath = path.join(HOST_PACKAGE_ROOT, "hermes");
const hermesPath = path.join(reactNativePath, "sdks", "node-api-hermes");
if (force && fs.existsSync(hermesPath)) {
await oraPromise(
fs.promises.rm(hermesPath, { recursive: true, force: true }),
Expand Down
Loading