feat(ses): Shim compatible with Hermes compiler #2334
Conversation
leotm
changed the title
|
ready for review ^ tackling the testing strategy separately in a follow-up PR, to keep this change small |
There was a problem hiding this comment.
This is indeed far more surgical than I expected.
I only request that we capture the async generator function instance in a single try{eval}catch in commons.js so we can just use if blocks to decide whether to include async generators in the various points you’re currently using try/catch.
I would like to make sure @erights looks at these changes as well.
Are you familiar with stacked PRs? You can propose the test changes in a PR based on this branch so they can be reviewed together. I would hesitate to approve code that doesn’t come with tests in the same merge, though I’m fine with reviewing them separately. Much depends on your workflow. One workflow that I like is individually reviewable commits in a single PR. That does require more commit grooming, and it looks like you intend to squash the 21 commits here when you merge. Another workflow is to create “stacked” PRs for each review artifact, then merge them top to bottom, so that ultimately the feature and its tests arrive in |
|
Glad to see this!
Hopefully tomorrow. |
Co-authored-by: legobeat <[email protected]>
Co-authored-by: legobeat <[email protected]>
| "./tools.js": "./tools.js", | ||
| "./assert-shim.js": "./assert-shim.js", | ||
| "./lockdown-shim.js": "./lockdown-shim.js", | ||
| "./compartment-shim.js": "./compartment-shim.js", | ||
| "./package.json": "./package.json" | ||
| }, | ||
| "scripts": { | ||
| "build": "node scripts/bundle.js", | ||
| "build:vanilla": "node scripts/bundle.js", | ||
| "build:hermes": "SES_BUILD_TYPE=hermes node scripts/bundle.js", |
There was a problem hiding this comment.
this will fail in powershell (if it matters)
consider util.parseArgs() instead of env vars
packages/ses/package.json
Outdated
| "test:hermes": "./scripts/hermes.sh", | ||
| "test:create-hermes-bin-symlinks": "./scripts/hermes-bin-symlinks.sh", |
There was a problem hiding this comment.
also not particularly windows-friendly 😄
| @@ -1,10 +1,14 @@ | |||
| /** @import {ModuleTransforms} from '../../compartment-mapper/types.js' */ | |||
There was a problem hiding this comment.
note: conflict with #2310; this would want to be SyncModuleTransforms instead.
also, if you're reaching in to compartment-mapper from ses (which it appears creates a cyclic dependency), it seems like compartment-mapper (or somewhere else) might want to be where this script lives.
Co-authored-by: Christopher Hiller <[email protected]>
Co-authored-by: Christopher Hiller <[email protected]>
|
|
||
| // Test for async generator function syntax support. | ||
| let AsyncGeneratorNewFunctionInstance; | ||
| try { | ||
| // Wrapping one in an new Function lets the `hermesc` binary file | ||
| // parse the Metro js bundle without SyntaxError, to generate the | ||
| // optimised Hermes bytecode bundle, when `gradlew` is called to | ||
| // assemble the release build APK for React Native prod Android apps. | ||
| // Delaying the error until runtime lets us customise lockdown behaviour. | ||
| AsyncGeneratorNewFunctionInstance = new FERAL_FUNCTION( | ||
| 'return (async function* AsyncGeneratorFunctionInstance() {})', | ||
| )(); | ||
| } catch (e) { | ||
| // @ts-expect-error ts(2339) Property 'jsEngine' does not exist on type 'Error'. However it exists on Hermes. | ||
| if (Error.prototype.jsEngine === 'hermes' && e.name === 'SyntaxError') { | ||
| // Swallows Hermes error `async generators are unsupported` at runtime. | ||
| // @ts-expect-error ts(2554) Expected 0 arguments, but got 1. It refers to the Web API Window object, but on Hermes we expect 1 argument. | ||
| // eslint-disable-next-line no-undef | ||
| print('SES: Skipping async generators, unsupported on Hermes'); | ||
| // Note: `console` is not a JS built-in, so Hermes engine throws: | ||
| // Uncaught ReferenceError: Property 'console' doesn't exist | ||
| // See: https://github.com/facebook/hermes/issues/675 | ||
| // However React Native provides a `console` implementation: | ||
| // https://github.com/facebook/react-native/blob/main/packages/react-native/Libraries/Core/InitializeCore.js | ||
| } else { | ||
| throw e; | ||
| } | ||
| } | ||
| export const AsyncGeneratorFunctionInstance = AsyncGeneratorNewFunctionInstance; |
There was a problem hiding this comment.
Would this work?
| // Test for async generator function syntax support. | |
| let AsyncGeneratorNewFunctionInstance; | |
| try { | |
| // Wrapping one in an new Function lets the `hermesc` binary file | |
| // parse the Metro js bundle without SyntaxError, to generate the | |
| // optimised Hermes bytecode bundle, when `gradlew` is called to | |
| // assemble the release build APK for React Native prod Android apps. | |
| // Delaying the error until runtime lets us customise lockdown behaviour. | |
| AsyncGeneratorNewFunctionInstance = new FERAL_FUNCTION( | |
| 'return (async function* AsyncGeneratorFunctionInstance() {})', | |
| )(); | |
| } catch (e) { | |
| // @ts-expect-error ts(2339) Property 'jsEngine' does not exist on type 'Error'. However it exists on Hermes. | |
| if (Error.prototype.jsEngine === 'hermes' && e.name === 'SyntaxError') { | |
| // Swallows Hermes error `async generators are unsupported` at runtime. | |
| // @ts-expect-error ts(2554) Expected 0 arguments, but got 1. It refers to the Web API Window object, but on Hermes we expect 1 argument. | |
| // eslint-disable-next-line no-undef | |
| print('SES: Skipping async generators, unsupported on Hermes'); | |
| // Note: `console` is not a JS built-in, so Hermes engine throws: | |
| // Uncaught ReferenceError: Property 'console' doesn't exist | |
| // See: https://github.com/facebook/hermes/issues/675 | |
| // However React Native provides a `console` implementation: | |
| // https://github.com/facebook/react-native/blob/main/packages/react-native/Libraries/Core/InitializeCore.js | |
| } else { | |
| throw e; | |
| } | |
| } | |
| export const AsyncGeneratorFunctionInstance = AsyncGeneratorNewFunctionInstance; | |
| const getAsyncGeneratorFunctionInstance = () => { | |
| // Test for async generator function syntax support. | |
| try { | |
| // Wrapping one in an new Function lets the `hermesc` binary file | |
| // parse the Metro js bundle without SyntaxError, to generate the | |
| // optimised Hermes bytecode bundle, when `gradlew` is called to | |
| // assemble the release build APK for React Native prod Android apps. | |
| // Delaying the error until runtime lets us customise lockdown behaviour. | |
| return new FERAL_FUNCTION( | |
| 'return (async function* AsyncGeneratorFunctionInstance() {})', | |
| )(); | |
| } catch (e) { | |
| // @ts-expect-error ts(2339) Property 'jsEngine' does not exist on type 'Error'. However it exists on Hermes. | |
| if (Error.prototype.jsEngine === 'hermes' && e.name === 'SyntaxError') { | |
| // Swallows Hermes error `async generators are unsupported` at runtime. | |
| // @ts-expect-error ts(2554) Expected 0 arguments, but got 1. It refers to the Web API Window object, but on Hermes we expect 1 argument. | |
| // eslint-disable-next-line no-undef | |
| print('SES: Skipping async generators, unsupported on Hermes'); | |
| // Note: `console` is not a JS built-in, so Hermes engine throws: | |
| // Uncaught ReferenceError: Property 'console' doesn't exist | |
| // See: https://github.com/facebook/hermes/issues/675 | |
| // However React Native provides a `console` implementation: | |
| // https://github.com/facebook/react-native/blob/main/packages/react-native/Libraries/Core/InitializeCore.js | |
| } else { | |
| throw e; | |
| } | |
| } | |
| export const AsyncGeneratorFunctionInstance = getAsyncGeneratorFunctionInstance(); |
Co-authored-by: legobeat <[email protected]>
Description
Add lockdown shim compatible with HermesMake current shim compatible with Hermes
for building React Native (RN) prod apps with SES
Generating the release AAB (Android App Bundle)
i.e.
npx react-native build-android --mode=releasevia RN CLIcalls Gradle's
bundleReleasetask under the hood (bundlebuild task onreleasevariant)which calls RNGP (React Native Gradle Plugin) React task
createBundleReleaseJsAndAssetsand failsafter Metro finishes writing the release bundle (bundle, sourcemaps, assets)
Gradle emits these Hermes errors
async functions are unsupportedasync arrow functions are unsupportedfacebook/hermes#1395async generators are unsupported(at runtime we can see both are
SyntaxErrors)Resulting in vague
java.lang.StackOverflowError (no error message)The try/catch approach testing language ft support via a new fn works
since RNGP no longer emits the Hermes errors in the task after Metro bundles
and we're conditionally using parts of SES compatible with the JS engine
The initial approach involved building a new shim via an env var
which involved a lot of duplicates
/srcfilesNb: clean before bundling to see changes reflected
i.e.
./gradlew clean :app:bundleReleaseNb:
async function* a() {};alone won't emit an errorbut using/referencing it and beyond
const b = a;willNb: eventually we hit RNGP BundleHermesCTask.kt > run > detectedHermesCommand > detectOSAwareHermesCommand from PathUtils.kt, which calls the Hermes compiler default command
hermesc- the path of the binary file, to create the optimised bytecode bundle to load/exec at runtimeTODO
./gradlew :app:createBundleReleaseJsAndAssets./gradlew :app:installRelease -PreactNativeArchitectures=arm64-v8ayarn <android/ios> --mode releaseFollow-up, CI testing options discussed
cd android && ./gradlew :app:bundleReleaseCI(macos): RN app test + SES, Xcode releasetest262:hermesscript (liketest262:xs)Security Considerations
Scaling Considerations
Documentation Considerations
Testing Considerations
Compatibility Considerations
Upgrade Considerations