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

metro-serializer-esbuild: "Transforming generator functions to the configured target environment ("es5") is not supported yet" #1743

Closed
2 of 4 tasks
jamonholmgren opened this issue Jul 14, 2022 · 12 comments · Fixed by #1812
Labels
bug Something isn't working documentation Improvements or additions to documentation enhancement New feature or request feature: metro This is related to Metro

Comments

@jamonholmgren
Copy link

jamonholmgren commented Jul 14, 2022

What happened?

I'm currently trying out @rnx-kit/metro-serializer-esbuild on Ignite and running into the following error:

Transforming generator functions to the configured target environment ("es5") is not supported yet

Current diff: https://github.com/infinitered/ignite/compare/rnx-kit-esbuild

I can't figure out where the target is configured for esbuild so I can change it to esnext or similar. Ideas?

Affected Package

@rnx-kit/metro-serializer-esbuild

Version

0.1.9

Which platforms are you seeing this issue on?

  • Android
  • iOS
  • macOS
  • Windows

System Information

System:
    OS: macOS 12.4
    CPU: (10) arm64 Apple M1 Max
    Memory: 17.55 GB / 64.00 GB
    Shell: 5.8.1 - /bin/zsh
  Binaries:
    Node: 14.19.1 - /opt/homebrew/opt/node@14/bin/node
    Yarn: 1.22.19 - /opt/homebrew/bin/yarn
    npm: 6.14.16 - /opt/homebrew/opt/node@14/bin/npm
    Watchman: 2022.03.21.00 - /opt/homebrew/bin/watchman
  Managers:
    CocoaPods: 1.11.3 - /opt/homebrew/bin/pod
  SDKs:
    iOS SDK:
      Platforms: DriverKit 21.4, iOS 15.5, macOS 12.3, tvOS 15.4, watchOS 8.5
    Android SDK: Not Found
  IDEs:
    Android Studio: 2021.2 AI-212.5712.43.2112.8512546
    Xcode: 13.4.1/13F100 - /usr/bin/xcodebuild
  Languages:
    Java: 11.0.14 - /Users/jh/.sdkman/candidates/java/current/bin/javac
  npmPackages:
    @react-native-community/cli: Not Found
    react: 17.0.2 => 17.0.2
    react-native: 0.68.2 => 0.68.2
    react-native-macos: Not Found
  npmGlobalPackages:
    *react-native*: Not Found


### Steps to Reproduce

Clone down the repo:

git clone https://github.com/infinitered/ignite.git

Switch to the branch

git checkout rnx-kit-esbuild

cd into the boilerplate folder
yarn
yarn build-ios
yarn build-android # if you want, same thing


### Code of Conduct

- [X] I agree to follow this project's Code of Conduct
@jamonholmgren jamonholmgren added the bug Something isn't working label Jul 14, 2022
@ghost ghost added the needs triage 🔍 label Jul 14, 2022
@afoxman
Copy link
Contributor

afoxman commented Jul 14, 2022

Thank you for filing this @jamonholmgren! I will explain what is going on, and then defer to @tido64 to decide what we can do about it.

Why is this happening?

We use esbuild to make tree shaking work with Metro.

We configure esbuild to target an "es5" runtime environment to ensure that we're compatible with Hermes. Hermes doesn't fully support es6 yet.

Esbuild doesn't fully support transpiling down to "es5", though. See here:

If you use a syntax feature that esbuild doesn't yet have support for transforming to your current language target, esbuild will generate an error where the unsupported syntax is used. This is often the case when targeting the es5 language version, for example, since esbuild only supports transforming most newer JavaScript syntax features to es6.

So this puts you in a bind. Effectively, you have to write your code in es5 for our tree shaking solution to work as it stands today.

What about ES6?

I tried changing the esbuild target to es6 (by hand, in node_modules) and it works just fine. es6 bundle is made. Ignite sample app loads it just fine (after a bit of hacking to make debug builds load from a file).

Why can't I use ES6? I don't care about Hermes.

This is where @tido64 comes in.

Should we expose a "target" option in the tree shaking config? e.g. "hermes" (implies es5), "es6", and maybe "esnext"?

Whatever is decided, we need to update the README and the guides which cover tree shaking to explain this limitation, what it looks like when you run into it, and how to work around it (or if you have to bail on the whole thing).

@afoxman afoxman added documentation Improvements or additions to documentation enhancement New feature or request feature: metro This is related to Metro and removed needs triage 🔍 labels Jul 14, 2022
@afoxman
Copy link
Contributor

afoxman commented Jul 14, 2022

esbuild-es5-errors

Attached picture of what esbuild emits when it runs into code it can't transform to es5 (for the docs).

@jamonholmgren
Copy link
Author

Thank you @afoxman!

I unfortunately do care about Hermes, as we are intending to turn it on by default in future Ignite releases.

Follow-up question: is the Hermes incompatibility with ES6 just some random edge cases, or is it a blocker? Studying the linked issue(s), it's hard to tell.

Secondly: could we transpile from esnext to ES5 after running esbuild, using Babel? So Babel would re-transpile the esbuild output to ES5, and that could be configurable if Hermes is enabled.

@afoxman
Copy link
Contributor

afoxman commented Jul 14, 2022

For (1), @tido64 will know.

For (2), I don't know how well it would work to transpile the whole bundle after-the-fact. Why not do it earlier, during the Metro run, via a babel plugin? I was able to get something working using this:

  presets: [
+    ["@babel/preset-env", { loose: true }],
     ["@rnx-kit/babel-preset-metro-react-native", { unstable_transformProfile: "esbuild" }],
  ],

You will need to pass --reset-cache to Metro the first time you run this.

I am a Babel Novice, so while this no longer makes esbuild/tree shaking upset, it may be doing horrible things to the bundle. But it demonstrates the approach, and from here, I'd consult a Babel Sage for more advice.

I did test it with the Ignite sample app, and things worked.

@tido64
Copy link
Member

tido64 commented Jul 15, 2022

As Adam mentioned, Hermes only implements selected ES6 features and lacks basic things like blocked-scope variables. If you want more details, I suggest you reach out to the Hermes folks on Discord.

As for providing target as a parameter, the plugin does support it and I thought I had documented it 😛 You can pass target as below:

 module.exports = makeMetroConfig({
   projectRoot: __dirname,
   serializer: {
+    customSerializer: MetroSerializer([], { target: "es5" }),
   },
   transformer: esbuildTransformerConfig,
 });

As of esbuild 0.14.49, you can also do:

 module.exports = makeMetroConfig({
   projectRoot: __dirname,
   serializer: {
+    customSerializer: MetroSerializer([], { target: "hermes0.70.0" }),
   },
   transformer: esbuildTransformerConfig,
 });

I haven't tested with hermes0.70.0 a lot so let us know how it works for you.

@tido64
Copy link
Member

tido64 commented Jul 15, 2022

Submitted a PR to document options.

Also, I wouldn't use @babel/preset-env as it includes a lot more than you need. For generators, you should include @babel/plugin-transform-regenerator instead.

@jamonholmgren
Copy link
Author

jamonholmgren commented Jul 22, 2022

Created a WIP PR to track over on Ignite (infinitered/ignite#1984). This will be part of our Ignite Maverick release, if we can get it to work reliably. Update 2022-08-10: I'm going to punt this to a future release, but still keep the WIP open.

@tido64
Copy link
Member

tido64 commented Jul 23, 2022

Created a WIP PR to track over on Ignite (infinitered/ignite#1984). This will be part of our Ignite Maverick release, if we can get it to work reliably.

That's awesome! Keep us posted.

Also, FYI: #1761

@ecreeth
Copy link

ecreeth commented Jul 28, 2022

These are my thoughts about this topic:

Clone the repo urban-enigma

TEST 1

  • set treeShake: true
  • add target: "es6" in the MetroSerializer build options with metro-serializer-esbuild configured in metro.config.js
  • This is going to throw the following error: Transforming generator functions to the configured target environment ("hermes0.7.0") is not supported yet
  • The target: "es6" is ignored due the following lines:
    if (treeShake) {
    metroConfig.serializer.customSerializer = MetroSerializerEsbuild(plugins);
    Object.assign(metroConfig.transformer, esbuildTransformerConfig);
    } else if (plugins.length > 0) {
    // MetroSerializer acts as a CustomSerializer, and it works with both
    // older and newer versions of Metro. Older versions expect a return
    // value, while newer versions expect a promise.
    //
    // Its return type is the union of both the value and the promise.
    // This makes TypeScript upset because for any given version of Metro,
    // it's one or the other. Not both.
    //
    // Since it can handle either scenario, just coerce it to whatever
    // the current version of Metro expects.
    metroConfig.serializer.customSerializer = MetroSerializer(
    plugins
    ) as SerializerConfigT["customSerializer"];

TEST 2

  • set treeShake: false
  • add target: "es6" in the MetroSerializer build options with metro-serializer-esbuild configured in metro.config.js
  • The esbuild bundler will be skipped and metro will do the serializer job

@tido64
Copy link
Member

tido64 commented Jul 29, 2022

As far as I know, it's due to this line in metro-react-native-babel-preset: https://github.com/facebook/metro/blob/cd8548a582f39b66c3683d21eeb4484d864fb018/packages/metro-react-native-babel-preset/src/configs/main.js#L164

regenerator is disabled when Hermes is enabled. This is why I suggested adding @babel/plugin-transform-regenerator earlier.

@ecreeth
Copy link

ecreeth commented Aug 3, 2022

As far as I know, it's due to this line in metro-react-native-babel-preset: https://github.com/facebook/metro/blob/cd8548a582f39b66c3683d21eeb4484d864fb018/packages/metro-react-native-babel-preset/src/configs/main.js#L164

regenerator is disabled when Hermes is enabled. This is why I suggested adding @babel/plugin-transform-regenerator earlier.

@tido64 the metro-react-native-babel-preset is not the issue here. The problem is that esbuild consider the following features as no soported in the target hermes0.7.0:

  • arrow
  • const-and-let
  • default-argument
  • generator
  • optional-catch-binding
  • optional-chain
  • rest-argument
  • template-literal

Test plan

  1. git clone https://github.com/infinitered/ignite.git
  2. git checkout rnx-kit-esbuild
  3. cd boilerplate
  4. bump @rnx-kit/metro-serializer-esbuild to 0.1.10
  5. yarn
  6. yarn build-ios
  7. ERROR: Transforming generator functions to the configured target environment ("hermes0.7.0") is not supported yet
  8. apply the fallowing patch to metro-serializer-esbuild:
+    supported: {
+       generator: true,
+   }
  1. Done!
info esbuild bundle size: 1323827
info Writing bundle output to:, ios/main.jsbundle
info Done writing bundle output
info Copying 18 asset files
info Done copying assets
Done in 10.66s.

🙏 hope that this help!

I think that the best way to go here is use `es6` as a `target` and add the supported and unsupported features:
supported: {
    // Hermes only supports these ES6 features.
    arrow: true,
    "array-spread": true,
    "default-argument": true,
    destructuring: true,
    "for-of": true,
    generator: true,
    "object-accessors": true,
    "object-extensions": true,
    "object-rest-spread": true,
    "optional-catch-binding": true,
    "optional-chain": true,
    "rest-argument": true,
    "template-literal": true,
    "logical-assignment": true,
    "nullish-coalescing": true,
    "nested-rest-binding": true,
    // Hermes doesn't support these ES6 features (needs more investigation).
    "arbitrary-module-namespace-names": false,
    "async-await": false,
    "async-generator": false,
    bigint: false,
    class: false,
    "class-field": false,
    "class-private-accessor": false,
    "class-private-brand-check": false,
    "class-private-field": false,
    "class-private-method": false,
    "class-private-static-accessor": false,
    "class-private-static-field": false,
    "class-private-static-method": false,
    "class-static-blocks": false,
    "class-static-field": false,
    "const-and-let": false,
    "dynamic-import": false,
    "exponent-operator": false,
    "export-star-as": false,
    "for-await": false,
     hashbang: false,
    "import-assertions": false,
    "import-meta": false,
    "new-target": false,
    "node-colon-prefix-import": false,
    "node-colon-prefix-require": false,
    "regexp-dot-all-flag": false,
    "regexp-lookbehind-assertions": false,
    "regexp-match-indices": false,
    "regexp-named-capture-groups": false,
    "regexp-sticky-and-unicode-flags": false,
    "regexp-unicode-property-escapes": false,
    "top-level-await": false,
    "typeof-exotic-object-is-object": false,
    "unicode-escapes": false,
},

@tido64
Copy link
Member

tido64 commented Aug 22, 2022

@ecreeth Thanks for the help. I was merely referring to why there is a diff when targeting Hermes, but I guess I left out the context 😆 In any case, I've submitted a PR to re-enable the features that are safe to include. I looked through the others that are partially implemented and I have decided not to enable them to avoid surprises.

@ghost ghost locked as resolved and limited conversation to collaborators Oct 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working documentation Improvements or additions to documentation enhancement New feature or request feature: metro This is related to Metro
Projects
None yet
4 participants