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

fix: use import for event-target-shim to support mjs #38628

Closed

Conversation

EvanBacon
Copy link
Contributor

Summary:

The React Native community almost exclusively adds mjs support with something like: config.resolver.sourceExts.push("mjs"); which causes js to be resolved before mjs. Mainstream bundlers will do the opposite, resolving mjs then js, unless bundling for Node.js environments.

event-target-shim has a .js and .mjs entry, when we attempt to implement community-standard resolution in Metro the app fails to open on iOS––providing no indication of what failed. The issue here is that the mjs exports for event-target-shim don't support module.exports = function() {}, so we need to update some of the imports in react-native (note that one of the imports to event-target-shim already uses import/export).

Discovery

For future readers––to discover this bug, I wrote a custom Metro resolver which printed a list of any mjs file that was resolved. In a basic app we observe the following:

/node_modules/react-native/Libraries/Core/setUpXHR.js > /node_modules/abort-controller/dist/abort-controller.mjs
/node_modules/react-native/Libraries/Network/XMLHttpRequest.js > /node_modules/event-target-shim/dist/event-target-shim.mjs
/node_modules/react-native/Libraries/WebSocket/WebSocket.js > /node_modules/event-target-shim/dist/event-target-shim.mjs
/node_modules/react-native/Libraries/Blob/FileReader.js > /node_modules/event-target-shim/dist/event-target-shim.mjs
/node_modules/abort-controller/dist/abort-controller.mjs > /node_modules/event-target-shim/dist/event-target-shim.mjs

In all cases the mjs files are resolved via react-native importing third-party packages, specifically abort-controller and event-target-shim. I modified the custom Metro resolver to ignore mjs resolution in different files until I found the problematic imports. This revealed that the exports were changing in event-target-shim between mjs and js.

Further, this was difficult to discover because the code that attempts to invoke an object as a function (error) is happening during the React Native networking setup. Ideally this JS code would be isolated from the user's bundler configuration and therefore impossible to break.

Changelog:

[GENERAL] [FIXED] - Update event-target-shim import to support Metro resolving mjs modules before js.

Test Plan:

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Expo Partner: Expo Partner Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Jul 25, 2023
@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,843,430 -13
android hermes armeabi-v7a 8,152,586 -12
android hermes x86 9,349,217 -18
android hermes x86_64 9,191,945 -11
android jsc arm64-v8a 9,456,815 -48
android jsc armeabi-v7a 8,637,942 -55
android jsc x86 9,539,897 -53
android jsc x86_64 9,783,195 -53

Base commit: e64756a
Branch: main

@facebook-github-bot
Copy link
Contributor

@TheSavior has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@github-actions
Copy link

This pull request was successfully merged by @EvanBacon in e37e530.

When will my fix make it into a release? | Upcoming Releases

@github-actions github-actions bot added the Merged This PR has been merged. label Jul 27, 2023
@EvanBacon EvanBacon deleted the @evanbacon/fix-event-target branch July 28, 2023 03:39
billnbell pushed a commit to billnbell/react-native that referenced this pull request Jul 29, 2023
Summary:
The React Native community almost exclusively adds `mjs` support with something like: `config.resolver.sourceExts.push("mjs");` which causes `js` to be resolved before `mjs`. Mainstream bundlers will do the opposite, resolving `mjs` then `js`, unless bundling for Node.js environments.

`event-target-shim` has a `.js` and `.mjs` entry, when we [attempt to implement _community-standard_ resolution in Metro](expo/expo#23528) the app fails to open on iOS––providing no indication of what failed. The issue here is that the `mjs` exports for `event-target-shim` don't support `module.exports = function() {}`, so we need to update some of the imports in `react-native` (note that one of the imports to `event-target-shim` already uses `import/export`).

### Discovery

For future readers––to discover this bug, I wrote a custom Metro resolver which printed a list of any `mjs` file that was resolved. In a basic app we observe the following:

```
/node_modules/react-native/Libraries/Core/setUpXHR.js > /node_modules/abort-controller/dist/abort-controller.mjs
/node_modules/react-native/Libraries/Network/XMLHttpRequest.js > /node_modules/event-target-shim/dist/event-target-shim.mjs
/node_modules/react-native/Libraries/WebSocket/WebSocket.js > /node_modules/event-target-shim/dist/event-target-shim.mjs
/node_modules/react-native/Libraries/Blob/FileReader.js > /node_modules/event-target-shim/dist/event-target-shim.mjs
/node_modules/abort-controller/dist/abort-controller.mjs > /node_modules/event-target-shim/dist/event-target-shim.mjs
```

In all cases the mjs files are resolved via `react-native` importing third-party packages, specifically `abort-controller` and `event-target-shim`. I modified the custom Metro resolver to ignore mjs resolution in different files until I found the problematic imports. This revealed that the exports were changing in `event-target-shim` between mjs and js.

Further, this was difficult to discover because the code that attempts to invoke an object as a function (error) is happening during the React Native networking setup. Ideally this JS code would be isolated from the user's bundler configuration and therefore impossible to break.

## Changelog:

[GENERAL] [FIXED] - Update `event-target-shim` import to support Metro resolving `mjs` modules before `js`.

<!-- Help reviewers and the release process by writing your own changelog entry.

Pick one each for the category and type tags:

[ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message

For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests

Pull Request resolved: facebook#38628

Test Plan:
- If you add `mjs` support to the `metro.config.js` file **before** js (`config.resolver.sourceExts.unshift("mjs");`), the project should be capable of starting.
- Usage with the default `metro.config.js` setup works as well.
- expo/expo#23528 works.

Reviewed By: NickGerleman

Differential Revision: D47816854

Pulled By: TheSavior

fbshipit-source-id: ebaf2e7a3ec02ae61effa004058589053601b766
billnbell pushed a commit to billnbell/react-native that referenced this pull request Jul 29, 2023
Summary:
The React Native community almost exclusively adds `mjs` support with something like: `config.resolver.sourceExts.push("mjs");` which causes `js` to be resolved before `mjs`. Mainstream bundlers will do the opposite, resolving `mjs` then `js`, unless bundling for Node.js environments.

`event-target-shim` has a `.js` and `.mjs` entry, when we [attempt to implement _community-standard_ resolution in Metro](expo/expo#23528) the app fails to open on iOS––providing no indication of what failed. The issue here is that the `mjs` exports for `event-target-shim` don't support `module.exports = function() {}`, so we need to update some of the imports in `react-native` (note that one of the imports to `event-target-shim` already uses `import/export`).

### Discovery

For future readers––to discover this bug, I wrote a custom Metro resolver which printed a list of any `mjs` file that was resolved. In a basic app we observe the following:

```
/node_modules/react-native/Libraries/Core/setUpXHR.js > /node_modules/abort-controller/dist/abort-controller.mjs
/node_modules/react-native/Libraries/Network/XMLHttpRequest.js > /node_modules/event-target-shim/dist/event-target-shim.mjs
/node_modules/react-native/Libraries/WebSocket/WebSocket.js > /node_modules/event-target-shim/dist/event-target-shim.mjs
/node_modules/react-native/Libraries/Blob/FileReader.js > /node_modules/event-target-shim/dist/event-target-shim.mjs
/node_modules/abort-controller/dist/abort-controller.mjs > /node_modules/event-target-shim/dist/event-target-shim.mjs
```

In all cases the mjs files are resolved via `react-native` importing third-party packages, specifically `abort-controller` and `event-target-shim`. I modified the custom Metro resolver to ignore mjs resolution in different files until I found the problematic imports. This revealed that the exports were changing in `event-target-shim` between mjs and js.

Further, this was difficult to discover because the code that attempts to invoke an object as a function (error) is happening during the React Native networking setup. Ideally this JS code would be isolated from the user's bundler configuration and therefore impossible to break.

## Changelog:

[GENERAL] [FIXED] - Update `event-target-shim` import to support Metro resolving `mjs` modules before `js`.

<!-- Help reviewers and the release process by writing your own changelog entry.

Pick one each for the category and type tags:

[ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message

For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests

Pull Request resolved: facebook#38628

Test Plan:
- If you add `mjs` support to the `metro.config.js` file **before** js (`config.resolver.sourceExts.unshift("mjs");`), the project should be capable of starting.
- Usage with the default `metro.config.js` setup works as well.
- expo/expo#23528 works.

Reviewed By: NickGerleman

Differential Revision: D47816854

Pulled By: TheSavior

fbshipit-source-id: ebaf2e7a3ec02ae61effa004058589053601b766
billnbell pushed a commit to billnbell/react-native that referenced this pull request Jul 29, 2023
Summary:
The React Native community almost exclusively adds `mjs` support with something like: `config.resolver.sourceExts.push("mjs");` which causes `js` to be resolved before `mjs`. Mainstream bundlers will do the opposite, resolving `mjs` then `js`, unless bundling for Node.js environments.

`event-target-shim` has a `.js` and `.mjs` entry, when we [attempt to implement _community-standard_ resolution in Metro](expo/expo#23528) the app fails to open on iOS––providing no indication of what failed. The issue here is that the `mjs` exports for `event-target-shim` don't support `module.exports = function() {}`, so we need to update some of the imports in `react-native` (note that one of the imports to `event-target-shim` already uses `import/export`).

### Discovery

For future readers––to discover this bug, I wrote a custom Metro resolver which printed a list of any `mjs` file that was resolved. In a basic app we observe the following:

```
/node_modules/react-native/Libraries/Core/setUpXHR.js > /node_modules/abort-controller/dist/abort-controller.mjs
/node_modules/react-native/Libraries/Network/XMLHttpRequest.js > /node_modules/event-target-shim/dist/event-target-shim.mjs
/node_modules/react-native/Libraries/WebSocket/WebSocket.js > /node_modules/event-target-shim/dist/event-target-shim.mjs
/node_modules/react-native/Libraries/Blob/FileReader.js > /node_modules/event-target-shim/dist/event-target-shim.mjs
/node_modules/abort-controller/dist/abort-controller.mjs > /node_modules/event-target-shim/dist/event-target-shim.mjs
```

In all cases the mjs files are resolved via `react-native` importing third-party packages, specifically `abort-controller` and `event-target-shim`. I modified the custom Metro resolver to ignore mjs resolution in different files until I found the problematic imports. This revealed that the exports were changing in `event-target-shim` between mjs and js.

Further, this was difficult to discover because the code that attempts to invoke an object as a function (error) is happening during the React Native networking setup. Ideally this JS code would be isolated from the user's bundler configuration and therefore impossible to break.

## Changelog:

[GENERAL] [FIXED] - Update `event-target-shim` import to support Metro resolving `mjs` modules before `js`.

<!-- Help reviewers and the release process by writing your own changelog entry.

Pick one each for the category and type tags:

[ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message

For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests

Pull Request resolved: facebook#38628

Test Plan:
- If you add `mjs` support to the `metro.config.js` file **before** js (`config.resolver.sourceExts.unshift("mjs");`), the project should be capable of starting.
- Usage with the default `metro.config.js` setup works as well.
- expo/expo#23528 works.

Reviewed By: NickGerleman

Differential Revision: D47816854

Pulled By: TheSavior

fbshipit-source-id: ebaf2e7a3ec02ae61effa004058589053601b766
EvanBacon added a commit to expo/expo that referenced this pull request May 30, 2024
# Why

- related facebook/react-native#38628

---------

Co-authored-by: Expo Bot <34669131+expo-bot@users.noreply.github.com>
EvanBacon added a commit to expo/expo that referenced this pull request Jun 10, 2024
# Why

- related facebook/react-native#38628

---------

Co-authored-by: Expo Bot <34669131+expo-bot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. p: Expo Partner: Expo Partner Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants