microsoft / rnx-kit

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

metro-serializer-esbuild throws [ERR_INVALID_ARG_TYPE] after bundling #2185

Closed vicary closed 1 year ago

vicary commented 1 year ago

What happened?

I was trying out @rnx-kit/metro-serializer-esbuild with my existing project, which uses React Native 0.67.

Following the installation guide, the first error I encountered is babel not recognising the preset option disableImportExportTransform. I skip ahead to the next step and updated my metro.config.js, but right at the end of bundling it throws the following error:

 BUNDLE  ./index.js ▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓ 100.0% (3875/3875)TypeError [ERR_INVALID_ARG_TYPE]: The "string" argument must be of type string or an instance of Buffer or ArrayBuffer. Received undefined
    at new NodeError (node:internal/errors:399:5)
    at Function.byteLength (node:buffer:738:11)
    at finish (myapps/node_modules/metro/src/Server.js:683:25)
    at Server.requestProcessor [as _processBundleRequest] (myapps/node_modules/metro/src/Server.js:569:7)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async Server._processRequest (myapps/node_modules/metro/src/Server.js:380:9)

Affected Package

@rnx-kit/metro-serializer-esbuild

Version

0.1.20

Which platforms are you seeing this issue on?

System Information

System:
    OS: macOS 13.1
    CPU: (8) arm64 Apple M1
    Memory: 674.42 MB / 16.00 GB
    Shell: 5.8.1 - /bin/zsh
  Binaries:
    Node: 19.6.0 - /private/var/folders/gw/lvpkpr9d0nbct414_k55p3vh0000gn/T/xfs-30629ce2/node
    Yarn: 3.3.1 - /private/var/folders/gw/lvpkpr9d0nbct414_k55p3vh0000gn/T/xfs-30629ce2/yarn
    npm: 9.4.0 - /opt/homebrew/bin/npm
    Watchman: 2023.02.06.00 - /opt/homebrew/bin/watchman
  Managers:
    CocoaPods: 1.11.3 - /usr/local/bin/pod
  SDKs:
    iOS SDK:
      Platforms: DriverKit 22.2, iOS 16.2, macOS 13.1, tvOS 16.1, watchOS 9.1
    Android SDK: Not Found
  IDEs:
    Android Studio: 2022.1 AI-221.6008.13.2211.9477386
    Xcode: 14.2/14C18 - /usr/bin/xcodebuild
  Languages:
    Java: 11.0.15 - /usr/bin/javac
  npmPackages:
    @react-native-community/cli: Not Found
    react: ^17.0.1 => 17.0.2 
    react-native: ^0.67.0 => 0.67.5 
    react-native-macos: Not Found
  npmGlobalPackages:
    *react-native*: Not Found

Steps to Reproduce

I cannot reproduce this with a freshly created React Native project.

I want to make a minimum repo, but my knowledge with metro is really limited.

Where shall I start?

Code of Conduct

tido64 commented 1 year ago

If this cannot be reproduced in a freshly created RN project, then it's most likely a setup issue. I noticed that you're using Node 19. Could try again with 18 or 16? Can you post your babel.config.js and metro.config.js here? Would also be great to know what versions of metro you have installed. You can run yarn why metro if you use Yarn.

vicary commented 1 year ago

@tido64 metro version is 0.66.2, sharing my config files below:

babel.config.js

module.exports = {
  plugins: [
    ['@babel/plugin-proposal-decorators', {legacy: true}],
    'transform-inline-environment-variables',
  ],
  presets: [
    '@babel/preset-typescript',
    'module:metro-react-native-babel-preset',
    // {disableImportExportTransform: true},
  ],
};

metro.config.js

const {makeMetroConfig} = require('@rnx-kit/metro-config');
const {
  MetroSerializer,
  esbuildTransformerConfig,
} = require('@rnx-kit/metro-serializer-esbuild');

module.exports = makeMetroConfig({
  serializer: {
    customSerializer: MetroSerializer,
  },
  transformer: esbuildTransformerConfig,
});
tido64 commented 1 year ago

Hey, from what I can see, you need to make the following changes:

babel.config.js

When passing options to a preset, you need to wrap both in an array like below:

 module.exports = {
   plugins: [
     ['@babel/plugin-proposal-decorators', {legacy: true}],
     'transform-inline-environment-variables',
   ],
   presets: [
-    '@babel/preset-typescript',
-    'module:metro-react-native-babel-preset',
-    // {disableImportExportTransform: true},
+    ['module:metro-react-native-babel-preset', {disableImportExportTransform: true}],
   ],
 };

metro.config.js

MetroSerializer is a function. You need to call it:

 const {makeMetroConfig} = require('@rnx-kit/metro-config');
 const {
   MetroSerializer,
   esbuildTransformerConfig,
 } = require('@rnx-kit/metro-serializer-esbuild');

 module.exports = makeMetroConfig({
   serializer: {
-    customSerializer: MetroSerializer,
+    customSerializer: MetroSerializer(),
   },
   transformer: esbuildTransformerConfig,
 });

I hope that helps.

vicary commented 1 year ago

Thanks for the reply.

With the configs updated, the bundle successfully built and sent to my testing device via USB debug. My testing device, which is an Android Tablet with Hermes enabled, now throws another error:

Compiling JS failed: 1103:37:identifier, '{' or '[' expected in binding pattern Buffer size: 17366933 starts with : 766172205f5f42554e444c455f535441 and has protection mode(s): r--p
Click to see screenshot ![image](https://user-images.githubusercontent.com/85772/217483936-3c09b27a-f6c4-49b7-a895-411259ae34c5.png)

What shall I do next?

tido64 commented 1 year ago

Did you pass JS or bytecode to Hermes? You can get a more legible error message compiling the JS with Hermes manually (docs), or switching temporarily back to JSC.

vicary commented 1 year ago

Sorry for the lack of knowledge on this front, I need some time to build the hermes cli.

Meanwhile I tried ran react-native bundle to see the final bundle, extracting ±2 lines at position 1103:37 below:

AppRegistry.registerComponent(appName, function () {
  return App;
},0,[1,499,526,527,1193,3874],"index.js");
'use strict';
var invariant = require('invariant');

Looks like valid JS to me so far.

tido64 commented 1 year ago

You shouldn't have to build hermes yourself:

% ls node_modules/hermes-engine/*/hermesc*
node_modules/hermes-engine/linux64-bin/hermesc
node_modules/hermes-engine/osx-bin/hermesc
node_modules/hermes-engine/win64-bin/hermesc.exe
vicary commented 1 year ago

Thanks. I tried running Hermes against two files but still fails, details below:

React Native entrypoint

Compiling index.js gives the following error:

$ node_modules/hermes-engine/osx-bin/hermesc -emit-binary -out test.hbc index.js
index.js:1:1: error: 'import' statement requires module mode
import {AppRegistry} from 'react-native';
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

The metro bundle

Guessing that Hermes don't know ESM syntax, tried using the bundle result above.

$ yarn react-native bundle --entry-file=index.js --platform=android --config=metro.config.js --bundle-output=bundle.js
$ node_modules/hermes-engine/osx-bin/hermesc -emit-binary -out test.hbc bundle.js
test.js:1660:33: error: invalid expression
export default new EventEmitter(,5,[6],"node_modules/react-native/Libraries/EventEmitter/RCTDeviceEventEmitter.js");
                                ^

Looks like Hermes doesn't allow skipping parameters like this, shall I update the esbuild config in metro.config.js to disable some specific features?

tido64 commented 1 year ago

I'm not sure what's going on, but can you add --reset-cache to react-native bundle … and try again?

Essentially, disableImportExportTransform: true tells Babel to not transform import/export statements to require(). This is to allow MetroSerializer to perform tree shaking before transforming them. The output bundle shouldn't have any import/export statements unless you're explicitly targeting ES6.

vicary commented 1 year ago

The import syntax is from our actual source code, I believe React Native's original babel config will translate that into require. So, in theory, react-native bundle should produce something can be understood by Hermes.

Adding --reset-cache resulting in the exact same js bundle. I tried to compile it into bytecode with Hermes, it fails with the same error. Hermes does not understand the syntax new EventEmitter(,5,...) at the first comma, where it tries to skip the first input.

I am guessing that esbuild may have an option to disable this and produces new EventEmitter(undefined, 5, ...) instead?

vicary commented 1 year ago

Upon further inspection on the bundle contents, besides multiple weird syntax of first comma (,, I can see the following syntax which should be illegal.

import '../Core/InitializeCore',27,[28],"node_modules/react-native/Libraries/ReactPrivate/ReactNativePrivateInitializeCore.js";

I reduced the entrypoint index.js to a simple function component () => <></> and try again.

Now Hermes throws the same error in my testing tablet:

test.js:1103:37: error: identifier, '{' or '[' expected in binding pattern
function _interopRequireDefault(obj,1,[],"node_modules/@babel/runtime/helpers/interopRequireDefault.js") {
                                    ^

It is somehow mixing function declarations with invocation syntax, I am lost...

Attaching the bundle here: bundle.js.zip

I can see that metro is trying to add ,number,[number],string) at random places, where string is a path to some js file.

tido64 commented 1 year ago

Do you have a minimal repro? I don't think we can investigate this any further with just the output bundle.

vicary commented 1 year ago

I'll try further reducing my working repo in the weekends.

vicary commented 1 year ago

@tido64 I removed as much irrelevant contents as possible in this repo https://github.com/vicary/metro-serializer-esbuild-mr, please let me know if you need anything else to help you pin down the issue.

tido64 commented 1 year ago

Hey, I couldn't build the Android app because of this:

> Could not get unknown property 'MYAPP_RELEASE_STORE_FILE' for SigningConfig$AgpDecorated_Decorated{name=release, storeFile=null, storePassword=null, keyAlias=null, keyPassword=null, storeType=pkcs12, v1SigningEnabled=true, v2SigningEnabled=true, enableV1Signing=null, enableV2Signing=null, enableV3Signing=null, enableV4Signing=null} of type com.android.build.gradle.internal.dsl.SigningConfig$AgpDecorated.

But I did manage to bundle and run hermesc on the bundle without issues. If you were trying to run dev server, you will need to modify your babel.config.js to not enable disableImportExportTransform. Something like:

diff --git a/babel.config.js b/babel.config.js
index b5a9f4c..bcda718 100644
--- a/babel.config.js
+++ b/babel.config.js
@@ -1,3 +1,4 @@
+const env = process.env.BABEL_ENV || process.env.NODE_ENV;
 module.exports = {
   plugins: [
     ['@babel/plugin-proposal-decorators', {legacy: true}],
@@ -7,7 +8,10 @@ module.exports = {
     '@babel/preset-typescript',
     [
       'module:metro-react-native-babel-preset',
-      {disableImportExportTransform: true},
+      {
+        disableImportExportTransform:
+          env === "production" && process.env["RNX_METRO_SERIALIZER_ESBUILD"],
+      },
     ],
   ],
 };

Hermes doesn't understand import/export statements, and the esbuild plugin does not run when in debug mode. Hence the checks.

Further, in metro.config.js, esbuildTransformerConfig is also incompatible with dev server. You can only set this line if you're bundling a release bundle. Unfortunately, the environment variable may not have been set by Metro at this point so you might have to set your own variables.

If you were using @rnx-kit/cli, we try to make sure these settings are correctly set since we have better control over both.

vicary commented 1 year ago

Hey, I couldn't build the Android app because of this:

Because I removed my release.keystore

metro.config.js, esbuildTransformerConfig is also incompatible with dev server

That explains a lot, it breaks because I was trying to use this plugin with a dev server.

Sorry for the confusion, and thanks for your help!