Closed adamchel closed 5 years ago
Hi,
Thanks for the detailed bug report! What is probably happening is that when collecting dependencies, Metro has no way of knowing that the require
call on the failing line is a real require
. This is because the closure is creating a local binding of it, and we cannot determine its value at build time. Imagine the following counter-example:
(function(require) {
require('my-foo-module');
})(console.log);
If we didn't check if require
is the global one, this one would be caught as a dependency, when it's not. Thus, the mechanism of dependency collection is made to just recognize a require
that is not shadowed by anything else.
There's obviously a trade-off in whether they should be considered or not, but we've experienced with other pre-built packages the opposite, and that's why we decided to go for a safer approach. The exact line where the check happens is here, precisely on the !path.scope.getBinding(name)
.
Metro is safely designed to avoid this issues, though. When outputting a bundle, all require
references are renamed to _$$_REQUIRE
so that other bundlers avoid recognizing these calls as dependencies. But unfortunately, that's not the case on all bundlers.
Specifically regarding the problem on the mongodb-stitch-react-native-sdk
package, the compatibility layer between CommonJS and UMD probably requires (never better said) some work to avoid passing require
as a local variable, when in the CommonJS world. This would fix the issue and let the library build.
Manually adding the dependencies will not work anyway, because Metro uses integer ids for modules when transpiling; but these require
s would remain unchanged when compiling (as they are not recognized as require
calls!).
Hi @mjesun, thanks for the quick response! Thanks also for the explanation, I wasn't too familiar with how metro works and your explanation and counterexample were very helpful. I was able to confirm that if we package our module as pure CommonJS, it works with Metro. However, we have been structuring our packages to share common code between our Browser, Node.js, and React Native packages (the shared code lives in https://www.npmjs.com/package/mongodb-stitch-core-sdk and its dependencies). This necessitated UMD so our shared code can work in many environments.
I appreciate and understand the tradeoff you've made in Metro, but it seems that having require
be bound to a local variable is fundamentally how AMD works, and also how UMD creates compatibility between AMD and CommonJS.
See:
The TypeScript compiler (which we use to build our modules) uses this template. As it currently stands, any module compiled by TypeScript to UMD (and presumably many other UMD modules) will not be supported by this dependency collection behavior.
Perhaps the check you mentioned here could be a little more intelligent? If a local variable binding named exports
also exists in the same scope, would that be sufficient evidence that this is in the context of a UMD module, and the require
call can be processed normally?
The check you mentioned could become something like this (I'm happy to open a pull request):
if (callee.isIdentifier({name}) && (!path.scope.getBinding(name) || (path.scope.getBinding("exports")))) {
visited.add(processRequireCall(path, state).node);
}
In the case of the TypeScript compiler, this would capture all of the cases (I tested a similar change in the Metro 0.38 package I've been working with and it fixed the issue), and it seems unlikely that both require
and exports
would be bound to local variables in a non-UMD/AMD context. Where have you seen a locally-bound require
causing issues?
@rafeca Can I get your opinion on that?
Thanks @adamchel for filing the issue and sorry for the delay answering.
The proposed solution would work fine for UMD modules, but can potentially break for other types of modules. For example, when a module has been built in a similar way than Metro (but without renaming the local variables):
__d(function(global, require, module, exports) {
require(0); // This is an "internal" require which should not be processed by Metro
});
I see two potential solutions for this issue:
$$__metro_require
). This way, you could have a babel plugin for UMD modules that renames their require
statements to $$__metro_require
.Hope this helps
Thanks for the suggestion, we ended up removing support for UMD in our SDKs, and we now only publish our modules as CommonJS or ES6 modules (https://github.com/mongodb/stitch-js-sdk/releases/tag/v4.1.0). This makes our SDK work properly with Metro and React Native. I'll leave this issue open since Metro still can't handle UMD modules generated by TypeScript, but our issue is resolved with the workaround of not using UMD. Feel free to close this if you don't plan on fixing it.
Do you want to request a feature or report a bug? Bug
What is the current behavior? I’m working on creating an NPM package (“mongodb-stitch-react-native-sdk”) that should be usable by React Native applications, and I’m running into an issue where the bundle produced by the metro packager has an immediate runtime error when requiring this module.
Once I start the metro bundler (I’ve been running
react-native run-android
in my project), and I run the app with my Android device, the bundling phase seems to complete successfully, but I get an error that saysUnknown named module: ‘mongodb-stitch-react-native-core’
, that seems to be coming frommetroRequire
.This “mongodb-stitch-react-native-sdk” module is simply a UMD module (compiled by a TypeScript compiler) that imports other UMD modules that exist in
node_modules
as dependencies. The index.js of the module looks like this:I’ve pointed out the line that causes the failure in the screenshot. The error seems to be occurring because the
mongodb-stitch-react-native-core
dependency doesn’t exist in theverboseNamesToModuleIds
object map that exists in Metro’srequire
polyfill.I’ve tried to include the “mongodb-stitch-react-native-core” and the "mongodb-stitch-core-sdk" dependencies directly instead of using this module, but then I get a similar error because its index.js files have lines of code that look like this:
This just gives me the same “Unknown named module” error, but for “./auth/internal/AuthInfo”.
Is Metro not properly bundling the UMD module dependencies, or am I doing something wrong?
When I use
create-react-native-app
, I'm able to use themongodb-stitch-react-native-sdk
package without any issues, so this may have been due to something introduced since metro 0.30.0, which is the version in use bycreate-react-native-app
.If the current behavior is a bug, please provide the steps to reproduce and a minimal repository on GitHub that we can
yarn install
andyarn test
.https://github.com/adamchel/MetroBundlerNotWorkingRepro
With Android SDKs and an Android device or simulator connected, clone repo, go to directory, and run
What is the expected behavior? The app should bundle and start successfully, and result in no runtime errors.
Please provide your exact Metro configuration and mention your Metro, node, yarn/npm version and operating system.
I've tried this with React Native 0.56.0 (latest release), which uses Metro 0.38.4. This is the version in the Github repo I've included with a reproduction of the issue.
I've also tried this with React Native 0.57.0-rc.0, which uses Metro 0.43.6, but I get a seemingly unrelated runtime error:
As I mentioned earlier, when I use
create-react-native-app
, I'm able to use themongodb-stitch-react-native-sdk
package without any issues, so this may have been due to something introduced since metro 0.30.0, which is the version in use bycreate-react-native-app
.