search

microsoft / rnx-kit

Modern, scalable tools. Exceptional developer experience.
https://microsoft.github.io/rnx-kit/
MIT License
1.53k stars 97 forks source link

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

Closed jamonholmgren closed 2 years ago

jamonholmgren commented 2 years ago

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?

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
afoxman commented 2 years ago

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 commented 2 years ago
esbuild-es5-errors

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

jamonholmgren commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

Created a WIP PR to track over on Ignite (https://github.com/infinitered/ignite/pull/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 commented 2 years ago

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: https://github.com/microsoft/rnx-kit/pull/1761

ecreeth commented 2 years ago

These are my thoughts about this topic:

Clone the repo urban-enigma

TEST 1

TEST 2

tido64 commented 2 years ago

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 commented 2 years ago

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:

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,
+   }
  9. 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: ```js 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 commented 2 years ago

@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.