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

backmerge #9

Merged
merged 7 commits into from
Dec 29, 2019
Merged

backmerge #9

merged 7 commits into from
Dec 29, 2019

Conversation

alanjoxa
Copy link
Owner

Summary

Test plan

rickhanlonii and others added 7 commits December 13, 2019 07:58
Reviewed By: gaearon

Differential Revision: D18942870

fbshipit-source-id: 5c29654fd8b15f48138dbb90736e33c008bdf3fc
Reviewed By: dsainati1

Differential Revision: D19042657

fbshipit-source-id: 04840f769207442be45e82cda39a7611784a6ce2
Reviewed By: bestander

Differential Revision: D18982385

fbshipit-source-id: 56ad452a51eb98f92aac5d395a60544499f35c35
Reviewed By: zackargyle, motiz88

Differential Revision: D19196946

fbshipit-source-id: 81c449d0ec4351e9bb296c07de771b0f64589010
Summary:
**Summary**

Fix the following warning:
> `{ parser: "babylon" }` is deprecated; we now treat it as `{ parser: "babel" }`

**Test plan**

No more warnings and tests are still green.
Pull Request resolved: #494

Differential Revision: D19234930

Pulled By: cpojer

fbshipit-source-id: 54352e77c8b001a6993b90b4190c3271dd4d4769
Summary:
**Summary**
When an absolute dependency path changes but the relative path stays the same (e.g. an extension change), DeltaBundler does not pick up the change.

This ultimately leads to a "no such file or directory" error when trying to require a dependency with the incorrect extension.

To repro, create a file `foo.js` which imports `bar.js`:
```
// foo.js
import `bar`
```
 1. initialize the graph via `incrementalBundler.initializeGraph()`
 2. rename `bar.js` -> `bar.ts`
 3. update the graph via `incrementalBundler.updateGraph()`
 4. notice that the resulting graph still has `bar.js` listed as a dependency

**Test plan**

After apply these changes, I confirmed the delta now includes 1 added file (`bar.ts`), 1 modified file (`foo.js`) and 1 deleted file (`bar.js`) (where as before it only included the 1 modified file). I also confirmed that the "no such file or directory" error we were seeing at Airbnb went away.
Pull Request resolved: #493

Differential Revision: D19234772

Pulled By: cpojer

fbshipit-source-id: 6732d0e3b86fe0a34e095381e57cb44afd63ddd5
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: #499

Differential Revision: D19234783

Pulled By: cpojer

fbshipit-source-id: 9fb44dda015391a07f25c2fc829fef07a51cce62
@alanjoxa alanjoxa merged commit e55a37a into alanjoxa:watch-fix Dec 29, 2019
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 this pull request may close these issues.

6 participants