Add watch parameter back to config for jest haste map configuration #497
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
- This is a partial revert of the commit - facebook@0d6c135#diff-75dc57ca2d6ab410090adb917784434f - This change is a no-op, but will enable use to turn off the file watch of jest haste map for building in the app. - Turning off the file watch will improve the speed of build and also will work fine in the cloud servers where we cannot easily configure fs.inotify.max_user_watches
Codecov Report
@@ Coverage Diff @@
## master #497 +/- ##
=======================================
Coverage 84.12% 84.12%
=======================================
Files 175 175
Lines 5852 5852
Branches 969 969
=======================================
Hits 4923 4923
Misses 818 818
Partials 111 111
Continue to review full report at Codecov.
|
|
Thank you for this PR. Instead of adding back this config option, could we instead change it so that Metro detects whether it is run via CI, and switches the watch flag based on that? I would much prefer that as a solution and there are packages like |
|
@cpojer I am not sure if In #491 I considered "building an offline bundle" as the signal for flipping off |
That is an excellent idea, because majority of the metro users(the react-native feature developers) don't have much idea about these metro configurations. And getting an error like @cpojer Thank you for that direction. I just looked at the I will update this PR to support this auto CI detection as well. |
Reviewed By: gaearon Differential Revision: D18942870 fbshipit-source-id: 5c29654fd8b15f48138dbb90736e33c008bdf3fc
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: facebook#494
Differential Revision: D19234930
Pulled By: cpojer
fbshipit-source-id: 54352e77c8b001a6993b90b4190c3271dd4d4769
…k#493) 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: facebook#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: facebook#499 Differential Revision: D19234783 Pulled By: cpojer fbshipit-source-id: 9fb44dda015391a07f25c2fc829fef07a51cce62
backmerge
|
Following up in a new PR : #503 , because this PR got some merge issues. |
facebook-github-bot
pushed a commit
that referenced
this pull request
Jan 6, 2020
#503) Summary: **Summary** *Issue : #355 *Follow up on my old PR : #497 The RN app build process is failing in CI servers due to the always-on file watch of metro. Mostly the CI servers will have a limit on no. of file watch at a time. This is a problem for RN apps with large no. of dependencies, and thus with a large no. of files in `node_modules`. Which cause the build to fail with `ENOSPC` error. This PR will detect the CI servers with [`ci-info`](https://github.com/watson/ci-info/) module and turn off the [file-watching of JestHasteMap in metro](https://github.com/facebook/metro/blob/f6314e43071ae498b41d9e5579ad4a13941a4baa/packages/metro/src/node-haste/DependencyGraph.js#L95). ~~Also in the [Metro documentation we have a `watch` field](https://facebook.github.io/metro/docs/en/configuration#watch), but it is not working since [this commit ](0d6c135#diff-75dc57ca2d6ab410090adb917784434f). So this PR will also make it working, so that people who wish to turn off the watch can do that with their metro config.~~ And this change is a no-op to current metro users, as it will maintain the default watch behavior. Also we can manually turn off the file watch by setting the `CI=true` environment variable. **Motivation** *Why this fix is required now ?* This file watching functionality will help in accelerating the local development, but that is not expected when we are building the project to release in our CI pipelines, where the build is happening in some docker containers or some ec2 servers, where sometimes we do not have access to set the `fs.inotify.max_user_watches`. So people try the workarounds like, npm script for deleting the folders in the node_modules which are not required in the bundling process. **Test plan** Completing the [metro pull request workflow](https://github.com/facebook/metro/blob/master/CONTRIBUTING.md#workflow-and-pull-requests). And I tested the functionality by creating a [sample RN app](https://github.com/alanjoxa/AwesomeProject) by applying this [patch](https://github.com/alanjoxa/AwesomeProject/tree/master/patches) to its metro dependencies in node-modules, as part of its [postinstall script](https://github.com/alanjoxa/AwesomeProject/blob/e52b92fd04d61e9ed8530d5fdf7f59657b1a8a2e/package.json#L11). It is working as expected. **Test simulation instructions** ``` git clone https://github.com/alanjoxa/AwesomeProject.git cd AwesomeProject npm install npm start ``` This will start a metro server and serve the bundles by watching all files in the project root. We can turn off the file watch by restarting it by `CI=true npm start`. A sample bundle can be triggered by making a GET request at `http://localhost:8081/index.bundle?platform=ios`. When in - `npm start` - Metro server will watch for all files in the project folder, including `node_modules` and apply any file changes to the bundle on the subsequent build request. - `CI=true npm start` - Will not watch for files, that means we need to restart the server for seeing the file changes. Pull Request resolved: #503 Differential Revision: D19286561 Pulled By: cpojer fbshipit-source-id: 68f99f746440ab2e199189bba92489a79e800f5d
facebook-github-bot
pushed a commit
that referenced
this pull request
Jan 10, 2020
Summary: **Summary** In this PR I am disabling file watching for bundle generation done through `runBuild`. A `watch` option is also added to `Server` in order to enable that (and also because `Server` is used by the React Native CLI instead of `runBuild`). The goal of this is to solve `ENOSPC` errors related to excessive file watcher use, particularly in CI environments. See: #355 Context: I previously attempted to fix this by resolving a regression in the existing `watch` config option: #491 Which was closed in favor of a similar PR: #497 Which was then re-created in: #503 (and landed) The above PR was modified to automatically set the `watch` value depending on whether it was built in a CI environment. There was a desire to remove the `watch` config option altogether. At the end, we concluded that there is still a need for a caller to turn off file watching explicitly, and not only through CI detection. Therefore, I'm threading it through the code as an option that is passed through programmatically and *not* through the config. Once/if this lands, I will update the React Native CLI to use this option. **Test plan** I used the rn-cli as a test harness of sorts. Invoked metro via `react-native start` - Observed that the `watch` flag was true through `console.log` Invoked metro via `react-native bundle` - Observed that `watch` was set to false. Relatively new to this project, so please let me know if I should be taking a look at anything else. Pull Request resolved: #507 Differential Revision: D19341662 Pulled By: cpojer fbshipit-source-id: d0430364d57fefe027f6b1f58f964d9765cda752
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
This PR will fix the regression caused by this commit
In the Metro documentation we have a
watchfield, which is not working after the above commit.We cannot simply revert that commit because it will set the default value of the watch field to
false, but the current default value istrue. So I created this PR by reverting that commit and setting its default value totrue. Which make this commit a no-op to current functionality, but fix the regression.Motivation
Why this fix is required now ?
Apart from fixing the bug, this enables react-native app developers to build there app in cloud servers, which has limit in no. of file watch at a time. This is a pain for people who have large dependencies, which causes the
node_modulesto have a large no. of files, which cause the build to fail withENOSPCerror.Test plan
Apart from the completing the metro pull request workflow
I tested it by applying this patch to a sample react-native feature and it is working as expected. Also, since it is almost a revert of one of the previous commit, it has less foreign changes.
The test feature which I created is available here - https://github.com/alanjoxa/AwesomeProject
Test simulation instructions
This will start a metro server, now you can change the watch property in that project's metro config, and see the difference, by making a build request at by a GET at
http://localhost:8081/index.bundle?platform=ios.When
watch : true- Metro server will watch for all files in the project folder, includingnode_modulesand apply any file changes to the bundle on the subsequent build request.watch: false- Will not watch for files, that means we need to restart the server for seeing the file changes.This file watching functionality will help in accelerating the local development, but that is not expected when we are building the project to release or in our CI pipelines, where the build is happening in some docker containers or some ec2 servers, where sometimes we do not have access to set the
fs.inotify.max_user_watches. So people try the workarounds like, npm script for deleting the folders in the node_modules which are not required in the bundling process.