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

resolver.unstable_enablePackageExports results in exception around interopRequireDefault #984

Closed
deodad opened this issue May 12, 2023 · 15 comments
Assignees

Comments

@deodad
Copy link

deodad commented May 12, 2023

New Version

0.72.RC-3

Old Version

na

Build Target(s)

io sim

Output of react-native info

yarn run v1.22.19
$ /Users/deodad/Repos/RN0720RC32/node_modules/.bin/react-native info
info Fetching system and libraries information...
System:
  OS: macOS 13.3.1
  CPU: (12) arm64 Apple M2 Pro
  Memory: 1.04 GB / 32.00 GB
  Shell:
    version: "5.9"
    path: /bin/zsh
Binaries:
  Node:
    version: 18.14.0
    path: /var/folders/1p/8plc0yx167q53qdktkcfd20m0000gn/T/yarn--1683918327548-0.8468867441091508/node
  Yarn:
    version: 1.22.19
    path: /var/folders/1p/8plc0yx167q53qdktkcfd20m0000gn/T/yarn--1683918327548-0.8468867441091508/yarn
  npm:
    version: 9.6.3
    path: ~/Library/Caches/fnm_multishells/15355_1683908029786/bin/npm
  Watchman: Not Found
Managers:
  CocoaPods:
    version: 1.12.1
    path: /var/folders/1p/8plc0yx167q53qdktkcfd20m0000gn/T/frum_15377_1683908029799/bin/pod
SDKs:
  iOS SDK:
    Platforms:
      - DriverKit 22.4
      - iOS 16.4
      - macOS 13.3
      - tvOS 16.4
      - watchOS 9.4
  Android SDK: Not Found
IDEs:
  Android Studio: Not Found
  Xcode:
    version: 14.3/14E222b
    path: /usr/bin/xcodebuild
Languages:
  Java: Not Found
  Ruby:
    version: 3.1.3
    path: /var/folders/1p/8plc0yx167q53qdktkcfd20m0000gn/T/frum_15377_1683908029799/bin/ruby
npmPackages:
  "@react-native-community/cli": Not Found
  react:
    installed: 18.2.0
    wanted: 18.2.0
  react-native:
    installed: 0.72.0-rc.3
    wanted: 0.72.0-rc.3
  react-native-macos: Not Found
npmGlobalPackages:
  "*react-native*": Not Found
Android:
  hermesEnabled: true
  newArchEnabled: false
iOS:
  hermesEnabled: true
  newArchEnabled: false

Done in 0.97s.

Issue and Reproduction Steps

npx react-native@latest init RN0720RC3 --version 0.72.0-rc.3
set resolver.unstable_enablePackageExports true in metro.config.js
yarn start

Unhandled JS Exception: _$$_REQUIRE(_dependencyMap[0], "(...)/helpers/interopRequireDefault") is not a function (it is Object)

TypeError: _$$_REQUIRE(_dependencyMap[0], "(...)/helpers/interopRequireDefault") is not a function (it is Object)
    at anonymous (http://localhost:8081/index.bundle?platform=ios&dev=true&minify=false&modulesOnly=false&runModule=true&app=org.reactjs.native.example.RN0720RC32:25075:104)
    at loadModuleImplementation (http://localhost:8081/index.bundle?platform=ios&dev=true&minify=false&modulesOnly=false&runModule=true&app=org.reactjs.native.example.RN0720RC32:328:14)
    at guardedLoadModule (http://localhost:8081/index.bundle?platform=ios&dev=true&minify=false&modulesOnly=false&runModule=true&app=org.reactjs.native.example.RN0720RC32:227:38)
    at metroRequire (http://localhost:8081/index.bundle?platform=ios&dev=true&minify=false&modulesOnly=false&runModule=true&app=org.reactjs.native.example.RN0720RC32:123:92)
    at anonymous (http://localhost:8081/index.bundle?platform=ios&dev=true&minify=false&modulesOnly=false&runModule=true&app=org.reactjs.native.example.RN0720RC32:25052:108)
    at loadModuleImplementation (http://localhost:8081/index.bundle?platform=ios&dev=true&minify=false&modulesOnly=false&runModule=true&app=org.reactjs.native.example.RN0720RC32:328:14)
    at guardedLoadModule (http://localhost:8081/index.bundle?platform=ios&dev=true&minify=false&modulesOnly=false&runModule=true&app=org.reactjs.native.example.RN0720RC32:227:38)
    at metroRequire (http://localhost:8081/index.bundle?platform=ios&dev=true&minify=false&modulesOnly=false&runModule=true&app=org.reactjs.native.example.RN0720RC32:123:92)
    at anonymous (http://localhost:8081/index.bundle?platform=ios&dev=true&minify=false&modulesOnly=false&runModule=true&app=org.reactjs.native.example.RN0720RC32:24992:14)
    at loadModuleImplementation (http://localhost:8081/index.bundle?platform=ios&dev=true&minify=false&modulesOnly=false&runModule=true&app=org.reactjs.native.example.RN0720RC32:328:14)
    at guardedLoadModule (http://localhost:8081/index.bundle?platform=ios&dev=true&minify=false&modulesOnly=false&runModule=true&app=org.reactjs.native.example.RN0720RC32:219:47)
    at metroRequire (http://localhost:8081/index.bundle?platform=ios&dev=true&minify=false&modulesOnly=false&runModule=true&app=org.reactjs.native.example.RN0720RC32:123:92)
    at global (http://localhost:8081/index.bundle?platform=ios&dev=true&minify=false&modulesOnly=false&runModule=true&app=org.reactjs.native.example.RN0720RC32:124286:4)

anonymous
    index.bundle?platform=ios&dev=true&minify=false&modulesOnly=false&runModule=true&app=org.reactjs.native.example.RN0720RC32:25075:104
loadModuleImplementation
    index.bundle?platform=ios&dev=true&minify=false&modulesOnly=false&runModule=true&app=org.reactjs.native.example.RN0720RC32:328:14
guardedLoadModule
    index.bundle?platform=ios&dev=true&minify=false&modulesOnly=false&runModule=true&app=org.reactjs.native.example.RN0720RC32:227:38
metroRequire
    index.bundle?platform=ios&dev=true&minify=false&modulesOnly=false&runModule=true&app=org.reactjs.native.example.RN0720RC32:123:92
anonymous
    index.bundle?platform=ios&dev=true&minify=false&modulesOnly=false&runModule=true&app=org.reactjs.native.example.RN0720RC32:25052:108
loadModuleImplementation
    index.bundle?platform=ios&dev=true&minify=false&modulesOnly=false&runModule=true&app=org.reactjs.native.example.RN0720RC32:328:14
guardedLoadModule
    index.bundle?platform=ios&dev=true&minify=false&modulesOnly=false&runModule=true&app=org.reactjs.native.example.RN0720RC32:227:38
metroRequire
    index.bundle?platform=ios&dev=true&minify=false&modulesOnly=false&runModule=true&app=org.reactjs.native.example.RN0720RC32:123:92
anonymous
    index.bundle?platform=ios&dev=true&minify=false&modulesOnly=false&runModule=true&app=org.reactjs.native.example.RN0720RC32:24992:14
loadModuleImplementation
    index.bundle?platform=ios&dev=true&minify=false&modulesOnly=false&runModule=true&app=org.reactjs.native.example.RN0720RC32:328:14
guardedLoadModule
    index.bundle?platform=ios&dev=true&minify=false&modulesOnly=false&runModule=true&app=org.reactjs.native.example.RN0720RC32:219:47
metroRequire
    index.bundle?platform=ios&dev=true&minify=false&modulesOnly=false&runModule=true&app=org.reactjs.native.example.RN0720RC32:123:92
global
    index.bundle?platform=ios&dev=true&minify=false&modulesOnly=false&runModule=true&app=org.reactjs.native.example.RN0720RC32:124286:4
@kelset
Copy link
Contributor

kelset commented May 15, 2023

that's because that option doesn't exists?

AFAIK there's only unstable_enablePackageExports , see https://facebook.github.io/metro/docs/package-exports

@cortinico
Copy link

that's because that option doesn't exists?
AFAIK there's only unstable_enablePackageExports , see facebook.github.io/metro/docs/package-exports

Correct 👍 closing this one

@deodad deodad changed the title resolver.unstable_enablePackageImports results in exception around interopRequireDefault resolver.unstable_enablePackageExports results in exception around interopRequireDefault May 15, 2023
@deodad
Copy link
Author

deodad commented May 15, 2023

this was actually a typo when writing the issue, the flag I am seeing this error with is [unstable_enablePackageExports](https://facebook.github.io/metro/docs/configuration/#unstable_enablepackageexports-experimental) not unstable_enablePackageImports.

Updated the issue, please reopen

@kelset
Copy link
Contributor

kelset commented May 15, 2023

@deodad can you provide the full metro config file for the repro? I would like to make sure it's not a misconfiguration

@deodad
Copy link
Author

deodad commented May 15, 2023

sure thing

const {getDefaultConfig, mergeConfig} = require('@react-native/metro-config');

/**
 * Metro configuration
 * https://facebook.github.io/metro/docs/configuration
 *
 * @type {import('metro-config').MetroConfig}
 */
const config = {
  resolver: {
    unstable_enablePackageExports: true,
  },
};

module.exports = mergeConfig(getDefaultConfig(__dirname), config);

@kelset
Copy link
Contributor

kelset commented May 15, 2023

thanks. Can confirm that this indeeds happen:

Screenshot 2023-05-15 at 17 29 52

cc @huntie, @motiz88 and @robhogan that's for you folks. Let us know once you have an idea on how to solve.

@kelset kelset reopened this May 15, 2023
@cortinico cortinico transferred this issue from facebook/react-native May 15, 2023
@robhogan robhogan self-assigned this May 15, 2023
@robhogan
Copy link
Contributor

@kelset I'll take a look at this tomorrow. Just to check, is that repro on a fresh react-native init project?

@motiz88
Copy link
Contributor

motiz88 commented May 15, 2023

I'm unable to take a close look at the moment, but I suspect this has something to do with e70ceef. My guess is that Babel generates code that assumes the import and require conditions will be asserted precisely for the respective kinds of dependencies, and never both at once like Metro does. So we end up requireing the version intended for use with import, and not doing any ESM interop unwrapping of the result. (Because this is the ESM interop unwrapping function.)

@motiz88
Copy link
Contributor

motiz88 commented May 16, 2023

If my suspicion above is correct: One approach could be to make the resolver assert either the import or require condition dynamically, based on the type of dependency seen in the AST in collectDependencies. This means also adding this distinction to each dependency's key (edge case: import 'a' and require('a') from the same file can resolve to different files!)

In practice we would end up using the require branch everywhere (because Babel lowers ESM imports to require by that point), except under experimentalImportSupport: true (note: used at Meta but not yet supported in OSS). Which means any packages that expose only an import path would always warn/fail under unstable_enablePackageExports: true until we roll out ESM support to OSS... So that's not exactly ideal.

@robhogan
Copy link
Contributor

robhogan commented May 16, 2023

In practice we would end up using the require branch everywhere (because Babel lowers ESM imports to require by that point)

The problem we'd have here is that effectively in OSS takes us back to facebook/react-native#36584, which was reverted because it breaks compatibility with ESM-only packages.

We've come back a few times to the idea that we should assert import as a last resort or fallback, maybe after default or only if resolution fails - such behaviour is definitely not within the exports spec, but it might be on a migration path.

@kelset
Copy link
Contributor

kelset commented May 17, 2023

@kelset Lorenzo Sciandra FTE I'll take a look at this tomorrow. Just to check, is that repro on a fresh react-native init project?

yes

@NickGerleman
Copy link
Contributor

We've come back a few times to the idea that we should assert import as a last resort or fallback, maybe after default or only if resolution fails - such behaviour is definitely not within the exports spec, but it might be on a migration path.

Is the issue that there are some previously working NPM packages which declare a main that is CommonJS, but exports that has only conditions for ESM and no other default?

If that is the issue, one more fallback strategy might be using main field based resolution if:

  1. Exports style resolution fails
  2. Exports style resolution would pass if we had conditions to allow ESM

@robhogan
Copy link
Contributor

robhogan commented May 21, 2023

@NickGerleman - Yeah, we could remove the import assertion and fall right back to non-exports resolution if exports resolution fails. It's fiddly though, partly because this is beyond just main/react-native/browser package entry points but also affects subpath resolution - re your condition 2., just because there's an entry in the exports map for a subpath with the import condition asserted, that doesn't necessarily match how it'd be resolved "traditionally", where the subpath is expected to literally match a path on disk (after appending extensions).

Indeed if we've reached the source file by successfully following the exports map, it might reasonably use subpath imports for its own dependencies that don't point to file locations and must be resolved with exports support - particularly if the dependencies are within a multi-package project like Babel. I haven't seen a concrete example of that, to be fair, but I feel like there are edge cases to whichever spec deviation we choose here.

Re the particular issue with interopRequireDefault, it's worth reiterating as @motiz88 pointed out, this is the module that allows const foo = require('foo') to work interchangeably with import foo from 'foo'. Maybe a surgical fix for that particular module is all we really need, and after that we're fine to assert import.

One possibility might simply to downgrade it - the problem export was only introduced in @babel/runtime@7.14.0 - compare @babel/runtime@17.13.7/package.json, where the CJS version takes precedence.

@robhogan
Copy link
Contributor

robhogan commented May 22, 2023

Another option, though a gnarly one that's probably breaking in some way - set the default property on CJS module exports within our loadModuleImplementation, and disable the interop layer on our commonjs transform with importInterop: 'none'. Bonus: one fewer function call and object allocation on requires, so potentially a runtime performance boost.

huntie added a commit to huntie/metro that referenced this issue May 23, 2023
Summary:
This is a workaround for facebook#984. It adds a specific exception in the resolver for `babel/runtime` to prevent asserting the `"import"` condition name on these modules (when using Package Exports). This is necessary so that the CJS version of these Babel helpers are resolved, which enable CJS/MJS interop for all other modules (given our current strategy of resolving both `"require"` and `"import"` in all other packages and using Babel-driven ESM compatibility).

This workaround is removable if/when any of:
- babel/babel#15643 is merged and updated in React Native.
- We implement dynamic handling of `"require"` and `"import"` conditions via ESM support in a future version of Metro.

Changelog: **[Experimental]** Fix `babel/runtime` issue when using Package Exports

Differential Revision: D46107056

fbshipit-source-id: 6bf32ccf412cb57a1705a6ae3ee6d8c35cb306d5
huntie added a commit to huntie/metro that referenced this issue May 24, 2023
Summary:
Pull Request resolved: facebook#990

This is a workaround for facebook#984. It adds a specific exception in the resolver for `babel/runtime` to prevent asserting the `"import"` condition name on these modules (when using Package Exports). This is necessary so that the CJS version of these Babel helpers are resolved, which enable CJS/MJS interop for all other modules (given our current strategy of resolving both `"require"` and `"import"` in all other packages and using Babel-driven ESM compatibility).

This workaround is removable if/when any of:
- babel/babel#15643 is merged and updated in React Native.
- We implement dynamic handling of `"require"` and `"import"` conditions via ESM support in a future version of Metro.

Changelog: **[Experimental]** Fix `babel/runtime` issue when using Package Exports

Differential Revision: D46107056

fbshipit-source-id: e3b8f453d98a45c7b10df1e6c2ff97b4f017529f
huntie added a commit to huntie/metro that referenced this issue May 24, 2023
Summary:
Pull Request resolved: facebook#990

This is a workaround for facebook#984. It adds a specific exception in the resolver for `babel/runtime` to prevent asserting the `"import"` condition name on these modules (when using Package Exports). This is necessary so that the CJS version of these Babel helpers are resolved, which enable CJS/MJS interop for all other modules (given our current strategy of resolving both `"require"` and `"import"` in all other packages and using Babel-driven ESM compatibility).

This workaround is removable if/when any of:
- babel/babel#15643 is merged and updated in React Native.
- We implement dynamic handling of `"require"` and `"import"` conditions via ESM support in a future version of Metro.

Changelog: **[Experimental]** Fix `babel/runtime` issue when using Package Exports

Reviewed By: motiz88

Differential Revision: D46107056

fbshipit-source-id: 5c34c5b3e1aaec16b998499401c81fadee34c30d
facebook-github-bot pushed a commit that referenced this issue May 24, 2023
Summary:
Pull Request resolved: #990

This is a workaround for #984. It adds a specific exception in the resolver for `babel/runtime` to prevent asserting the `"import"` condition name on these modules (when using Package Exports). This is necessary so that the CJS version of these Babel helpers are resolved, which enable CJS/MJS interop for all other modules (given our current strategy of resolving both `"require"` and `"import"` in all other packages and using Babel-driven ESM compatibility).

This workaround is removable if/when any of:
- babel/babel#15643 is merged and updated in React Native.
- We implement dynamic handling of `"require"` and `"import"` conditions via ESM support in a future version of Metro.

Changelog: **[Experimental]** Fix `babel/runtime` issue when using Package Exports

Reviewed By: motiz88

Differential Revision: D46107056

fbshipit-source-id: 06c18de031458d6016ec7cf4101518e8bc1a174a
@huntie
Copy link
Member

huntie commented May 25, 2023

Resolved in https://github.com/facebook/metro, shipping today in Metro 0.76.5 and will also be updated in React Native CLI today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants