feat(ses): Shim compatible with Hermes compiler#2334
Conversation
|
ready for review ^ tackling the testing strategy separately in a follow-up PR, to keep this change small |
kriskowal
left a comment
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. |
kriskowal
left a comment
There was a problem hiding this comment.
I’m poised to approve these changes. The functional changes look good, with some polish desired.
I verified the Hermes test locally. It’s a great start.
Blocking: Please choose lexical compartment instead of this. I disprefer functionBind used on an arrow function as that is confusing and I believe unnecessary. Do let me know if there’s more to that.
Blocking: Please clear up the TODOs in the test. Those should either be DONETO or further work captured as a link to an issue.
Blocking: Please use the new reporters for logging SES Skipping async generators. The reporter is configurable and can paper over whether console or print capabilities are present in the host environment.
Not blocking: I would like a SES-specific lint rule, like the we have for @endo/polymorphic*, so that we get a lint error if we use this inside an arrow function, since evidently that will break the Hermes version.
| const { execute, exportsProxy } = link( | ||
| privateFields, | ||
| moduleAliases, | ||
| compartment, |
There was a problem hiding this comment.
I see that this changes this to the lexical compartment. That seems to obviate the functionBind. But also, async arrow functions do not rebind this. Is this change needed because of a the shift in semantics caused by the transformation? Do we need both accommodations or just one?
My preference would be to just use the compartment named in lexical scope since that doesn’t require the async function transform for explanation.
There was a problem hiding this comment.
the change is needed since the current hermes transform only replaces the async arrow fn with a fn expression with babel (without modifying this)
There was a problem hiding this comment.
without functionBind our 4 (compartment-mapper) optionalDependencies/esm tests fail
(@naugtur do you remember the exact reason)
compartment-mapper
optional › optionalDependencies/esm / loadLocation
test/optional.test.js:25
24: const result = await tryOptionalDeps();
25: t.deepEqual(
26: Reflect.ownKeys(result.exports),
expected exports
Difference (- actual, + expected):
[
+ 'alpha',
+ 'beta',
]
› scaffold.knownArchiveFailure (file://test/optional.test.js:25:7)
optional › optionalDependencies/esm / mapNodeModules / importFromMap
test/optional.test.js:25
24: const result = await tryOptionalDeps();
25: t.deepEqual(
26: Reflect.ownKeys(result.exports),
expected exports
Difference (- actual, + expected):
[
+ 'alpha',
+ 'beta',
]
› scaffold.knownArchiveFailure (file://test/optional.test.js:25:7)
optional › optionalDependencies/esm / mapNodeModules / loadFromMap / import
test/optional.test.js:25
24: const result = await tryOptionalDeps();
25: t.deepEqual(
26: Reflect.ownKeys(result.exports),
expected exports
Difference (- actual, + expected):
[
+ 'alpha',
+ 'beta',
]
› scaffold.knownArchiveFailure (file://test/optional.test.js:25:7)
optional › optionalDependencies/esm / importLocation
test/optional.test.js:25
24: const result = await tryOptionalDeps();
25: t.deepEqual(
26: Reflect.ownKeys(result.exports),
expected exports
Difference (- actual, + expected):
[
+ 'alpha',
+ 'beta',
]
› scaffold.knownArchiveFailure (file://test/optional.test.js:25:7)
─
4 tests failed
</details>Co-authored-by: Saleh Abdel Motaal <849613+SMotaal@users.noreply.github.com>
Co-authored-by: legobeat <109787230+legobeat@users.noreply.github.com>
Co-authored-by: naugtur <naugtur@gmail.com>
endo sync: the current error in the transform that provides feedback in CI is adequate |
kriskowal
left a comment
There was a problem hiding this comment.
One last round. Let’s obviate functionBind again, and also take on the explicit devDependencies on the the same versions of Babel used elsewhere in the project.
| "eslint-config-prettier": "^9.1.0", | ||
| "eslint-plugin-eslint-comments": "^3.2.0", | ||
| "eslint-plugin-import": "^2.29.1", | ||
| "hermes-engine-cli": "^0.12.0", |
There was a problem hiding this comment.
Let’s add explicit dependencies on the relevant Babel packages.
There was a problem hiding this comment.
note: added the same versions to packages/ses for parity
endo/packages/module-source/package.json
Lines 44 to 50 in f71479f
endo/packages/evasive-transform/package.json
Lines 80 to 85 in f71479f
as devDepencencies since they're not used in any packages/ses/src code
only packages/ses/scripts/hermes-transforms.js to bundle ses itself
|
I think there is a risk under maintenance that we add a new syntactic async generator in the SES-shim, that would be unexpectedly transformed under Hermes. Let's add a lint rule that warn on async generator usages that Hermes requires special handling, and silence the lint warning in the EvalError fallback case, which has the special handling. |
added to tracking issue under 'Non blocking' header |
kriskowal
left a comment
There was a problem hiding this comment.
Just a nit. Otherwise, good to go.
Description
Add lockdown shim compatible with HermesMake current shim compatible with Hermes compiler
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 Hermes 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 (avoid cache)
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