Pass the original module name to custom resolver #499
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We are using a custom resolver for our custom module resolution logic, and we got an issue where the module name passed to our custom resolver is not the actual name of the module in the node-modules. Our investigation find that the metro's resolver will change the original module name as per the redirect defined in "react-native" or "browser" field of the package.json of that requiring package. I found that, by just adding the original module name to the list of arguments passed to the custom resolver will solve the problem without affecting any of its existing users. Also this will give the custom resolver a better context on the required module, which the custom resolver can handle in a better way.
Codecov Report
@@ Coverage Diff @@
## master #499 +/- ##
=======================================
Coverage 84.46% 84.46%
=======================================
Files 173 173
Lines 5820 5820
Branches 961 961
=======================================
Hits 4916 4916
Misses 801 801
Partials 103 103
Continue to review full report at Codecov.
|
facebook-github-bot
added
the
CLA Signed
There was a problem hiding this comment.
@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
alanjoxa
added a commit
to alanjoxa/metro
that referenced
this pull request
Dec 29, 2019
Summary: **Summary** We are using a custom resolver for our custom module resolution logic, and we got an issue where the module name passed to our custom resolver is not the actual name of the module in the node-modules. Our investigation find that the metro's resolver will change the [original module name](https://github.com/facebook/metro/blob/master/packages/metro-resolver/src/resolve.js#L50) as per the redirect defined in "react-native" or "browser" field of the package.json of that requiring package. Eg. [here](https://github.com/aws/aws-sdk-js/blob/master/package.json#L66) I found that, by just adding the original module name to the list of arguments passed to the custom resolver will solve the problem without affecting any of its existing users/functionalities. Also this will give the custom resolver a better context on the required module, which the custom resolver can handle in a better way. We came to this fix by the investigation on the bug we faced, but we found it beneficial for anyone who wanted to use a custom resolver with metro. **Test plan** We are using this the change as a [patch](https://www.npmjs.com/package/patch-package) in our package for the past few months, so it is throughly tested for its newly added argument. Also I created a test feature which is available here - https://github.com/alanjoxa/AwesomeProject/tree/resolver Test simulation instructions ``` git clone https://github.com/alanjoxa/AwesomeProject.git git checkout resolver cd AwesomeProject npm install npm start ``` This will start a metro server at 8081. The metro config of this package has a [custom resolver](https://github.com/alanjoxa/AwesomeProject/blob/resolver/metro.config.js#L12), which is just intercepting all resolve request for the demo of this use case. Also this package [depend on the aws-sdk](https://github.com/alanjoxa/AwesomeProject/blob/resolver/package.json#L14) package which has a [react-native specific redirect defined](https://github.com/aws/aws-sdk-js/blob/master/package.json#L61). Now on any bundle request to this server - the metro will call the custom resolver for the modules required in the package and it will pass the requested module name alone with the context and platform . But without my patch, the original module name of the redirected modules will be unknown to the custom resolver. Eg: in place of [xml2js](https://github.com/aws/aws-sdk-js/blob/master/package.json#L66), the custom resolver will get it as "./dist/xml2js.js" Pull Request resolved: facebook#499 Differential Revision: D19234783 Pulled By: cpojer fbshipit-source-id: 9fb44dda015391a07f25c2fc829fef07a51cce62
alanjoxa
added a commit
to alanjoxa/metro
that referenced
this pull request
Dec 29, 2019
Summary: **Summary** We are using a custom resolver for our custom module resolution logic, and we got an issue where the module name passed to our custom resolver is not the actual name of the module in the node-modules. Our investigation find that the metro's resolver will change the [original module name](https://github.com/facebook/metro/blob/master/packages/metro-resolver/src/resolve.js#L50) as per the redirect defined in "react-native" or "browser" field of the package.json of that requiring package. Eg. [here](https://github.com/aws/aws-sdk-js/blob/master/package.json#L66) I found that, by just adding the original module name to the list of arguments passed to the custom resolver will solve the problem without affecting any of its existing users/functionalities. Also this will give the custom resolver a better context on the required module, which the custom resolver can handle in a better way. We came to this fix by the investigation on the bug we faced, but we found it beneficial for anyone who wanted to use a custom resolver with metro. **Test plan** We are using this the change as a [patch](https://www.npmjs.com/package/patch-package) in our package for the past few months, so it is throughly tested for its newly added argument. Also I created a test feature which is available here - https://github.com/alanjoxa/AwesomeProject/tree/resolver Test simulation instructions ``` git clone https://github.com/alanjoxa/AwesomeProject.git git checkout resolver cd AwesomeProject npm install npm start ``` This will start a metro server at 8081. The metro config of this package has a [custom resolver](https://github.com/alanjoxa/AwesomeProject/blob/resolver/metro.config.js#L12), which is just intercepting all resolve request for the demo of this use case. Also this package [depend on the aws-sdk](https://github.com/alanjoxa/AwesomeProject/blob/resolver/package.json#L14) package which has a [react-native specific redirect defined](https://github.com/aws/aws-sdk-js/blob/master/package.json#L61). Now on any bundle request to this server - the metro will call the custom resolver for the modules required in the package and it will pass the requested module name alone with the context and platform . But without my patch, the original module name of the redirected modules will be unknown to the custom resolver. Eg: in place of [xml2js](https://github.com/aws/aws-sdk-js/blob/master/package.json#L66), the custom resolver will get it as "./dist/xml2js.js" Pull Request resolved: facebook#499 Differential Revision: D19234783 Pulled By: cpojer fbshipit-source-id: 9fb44dda015391a07f25c2fc829fef07a51cce62
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
We are using a custom resolver for our custom module resolution logic,
and we got an issue where the module name passed to our custom resolver
is not the actual name of the module in the node-modules.
Our investigation find that the metro's resolver will change the original
module name as per the redirect defined in "react-native" or "browser"
field of the package.json of that requiring package. Eg. here
I found that, by just adding the original module name to the list of
arguments passed to the custom resolver will solve the problem without
affecting any of its existing users/functionalities. Also this will give the custom
resolver a better context on the required module, which the custom
resolver can handle in a better way.
We came to this fix by the investigation on the bug we faced, but we found it beneficial for anyone who wanted to use a custom resolver with metro.
Test plan
We are using this the change as a patch in our package for the past few months, so it is throughly tested for its newly added argument.
Also I created a test feature which is available here - https://github.com/alanjoxa/AwesomeProject/tree/resolver
Test simulation instructions
This will start a metro server at 8081.
The metro config of this package has a custom resolver, which is just intercepting all resolve request for the demo of this use case. Also this package depend on the aws-sdk package which has a react-native specific redirect defined. Now on any bundle request to this server - the metro will call the custom resolver for the modules required in the package and it will pass the requested module name alone with the context and platform . But without my patch, the original module name of the redirected modules will be unknown to the custom resolver.
Eg: in place of xml2js, the custom resolver will get it as "./dist/xml2js.js"