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(cli): allow Hermes to be run post-bundle #2423

Merged
merged 1 commit into from
Sep 28, 2023
Merged

feat(cli): allow Hermes to be run post-bundle #2423

merged 1 commit into from
Sep 28, 2023

Conversation

tido64
Copy link
Member

@tido64 tido64 commented May 19, 2023

Description

Run hermesc after the bundle has been created.

Test plan

Enable Hermes:

diff --git a/packages/test-app/package.json b/packages/test-app/package.json
index bf87fe69..07b4cd20 100644
--- a/packages/test-app/package.json
+++ b/packages/test-app/package.json
@@ -141,7 +141,8 @@
           "android": {
             "bundleOutput": "dist/main+esbuild.android.bundle",
             "sourcemapOutput": "dist/main+esbuild.android.bundle.map",
-            "assetsDest": "dist/res"
+            "assetsDest": "dist/res",
+            "hermes": true
           },
           "ios": {
             "bundleOutput": "dist/main+esbuild.ios.jsbundle",

Bundle:

cd packages/test-app
yarn build --dependencies
yarn bundle+esbuild --dev false

Verify that hermesc was only run for Android:

% ls dist
total 22960
drwxr-xr-x  3 tido  staff       96 26 sep 12:03 assets/
-rw-r--r--  1 tido  staff   631864 26 sep 12:04 main+esbuild.android.bundle
-rw-r--r--  1 tido  staff   599720 26 sep 12:04 main+esbuild.android.bundle.hbc
-rw-r--r--  1 tido  staff   477992 26 sep 12:04 main+esbuild.android.bundle.hbc.map
-rw-r--r--  1 tido  staff  3067887 26 sep 12:04 main+esbuild.android.bundle.map
-rw-r--r--  1 tido  staff   625665 26 sep 12:04 main+esbuild.ios.jsbundle
-rw-r--r--  1 tido  staff  3045528 26 sep 12:04 main+esbuild.ios.jsbundle.map
-rw-r--r--  1 tido  staff   627559 26 sep 12:05 main+esbuild.windows.bundle
-rw-r--r--  1 tido  staff  3062400 26 sep 12:05 main+esbuild.windows.bundle.map
drwxr-xr-x  3 tido  staff       96 26 sep 12:03 res/

Also verify that:

  • Moving "hermes": true up to the "global" area enables Hermes for all platforms
  • "hermes": { "flags": ["-out", "index.jsbundle"] } (and other options) is accepted

@github-actions github-actions bot added the feature: cli This is related to CLI label May 19, 2023
@tido64 tido64 force-pushed the tido/cli/hermes branch 3 times, most recently from 8c6d1dd to e6a024e Compare May 22, 2023 07:56
@tido64 tido64 linked an issue May 22, 2023 that may be closed by this pull request
@tido64 tido64 force-pushed the tido/cli/hermes branch 3 times, most recently from a2ffd10 to c417500 Compare May 22, 2023 08:47
@renchap
Copy link
Contributor

renchap commented May 22, 2023

  • Do we still want to keep the .js output?

This might be a good idea, for tools like Sentry. I am not sure if they can work with only the sourcemap.

@tido64 tido64 force-pushed the tido/cli/hermes branch 3 times, most recently from bcc1eb6 to 39d861f Compare May 23, 2023 09:04
packages/cli/src/bundle.ts Outdated Show resolved Hide resolved
@tido64 tido64 force-pushed the tido/cli/hermes branch 2 times, most recently from 49525ad to 96ba0d3 Compare September 26, 2023 10:59
@tido64 tido64 marked this pull request as ready for review September 26, 2023 11:33
Copy link
Contributor

@kelset kelset left a comment

Choose a reason for hiding this comment

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

LGTM

tested locally (all various options that needed testing) and all worked as expected 👍

@tido64 tido64 enabled auto-merge (squash) September 26, 2023 17:08
@tido64 tido64 merged commit 839ba0e into main Sep 28, 2023
@tido64 tido64 deleted the tido/cli/hermes branch September 28, 2023 05:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature: cli This is related to CLI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Plan for supporting Hermes in rnx-bundle and rnx-start
3 participants