-
Notifications
You must be signed in to change notification settings - Fork 24.5k
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
Fix local-cli assetRegistryPath and middlewares #20162
Conversation
Summary: Scope of the diff: 1. Middleware `react-native-github/local-cli` and `react-native-internal-cli` uses a very similar set of middlewares (internal cli extends github version), so I decided to move it to a standalone file (middleware manager) in order to remove duplications and increase readability. 2. Types Seems that after Flow upgrade to version 0.68 there were many type issues to resolve, so all of them were auto-mocked. This is fine, but I'd like to see Flow assists me with `Metro.createServer` -> `Metro.runServer` migration. Hence, I decided to resolve flow mocks, related to runServer. 3. `runServer` signature In `react-native-github` repo I cleaned up `runServer` signature by removing `startCallback` and `readyCallback` from the function parameters and moved them to `runServer` instead. 4. Replace `createServer` by `runServer` In `react-native-github` repo, `createServer` has been replaced by `runServer`. __Some of arguments are not mapped__. Note that this diff will partially break argument mapping. This is intentional. @[100000044482482:ives] will fix it with a new config package. Reviewed By: mjesun Differential Revision: D8711717 fbshipit-source-id: a843ab576360ff7242099910d8f25a9cb0a388c0
CI is back to green with this :) |
I found more issues with remote debugging because we don't attach the websocket proxy, looking into a fix. |
7d55ff0
to
bb1acc3
Compare
Looks like hmr is not working also 🙃 |
A lot of config keys were on the wrong object, hmr now works. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hramos is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
I tried to merge this pull request into the Facebook internal repo but some checks failed. To unblock yourself please check the following: Does this pull request pass all open source tests on GitHub? If not please fix those. Does the code still apply cleanly on top of GitHub master? If not can please rebase. In all other cases this means some internal test failed, for example a part of a fb app won't work with this pull request. I've added the Import Failed label to this pull request so it is easy for someone at fb to find the pull request and check what failed. If you don't see anyone comment in a few days feel free to comment mentioning one of the core contributors to the project so they get a notification. |
@@ -55,21 +59,32 @@ async function runServer(args: Args, config: ConfigT) { | |||
const serverInstance = await Metro.runServer({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error ---------- local-cli/server/runServer.js:59:48
Cannot call `Metro.runServer` with object literal bound to the first parameter because property `watch` is missing in `RunServerOptions` [1] but exists in object literal [2].
v
59| const serverInstance = await Metro.runServer({
60| config: {
61| ...config,
:
77| watch: !args.nonPersistent,
78| });
--^
References:
metro/packages/metro/src/index.js:201:4
201| }: RunServerOptions) => {
^^^^^^^^^^^^^^^^ [1]
v
59| const serverInstance = await Metro.runServer({
60| config: {
61| ...config,
:
77| watch: !args.nonPersistent,
78| });
--^ [2]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A Flow error is preventing the internal diff from landing.
@hramos Oh right, we ignore metro flow types in OSS so I it must be why I missed it. Working on a fix. |
The |
@hramos Should be good to land now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hramos is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
This pull request was closed by @janicduplessis in f05943d. Once this commit is added to a release, you will see the corresponding version tag below the description at f05943d. If the commit has a single |
Summary: This fixes some regressions with local-cli introduced in c4a66a89a28152cd4b81e2b0f80ab3aac34d6872. - We didn't pass `assetRegistryPath` which caused the following error when loading the bundle: ``` error: bundling failed: Error: Unable to resolve module `missing-asset-registry-path` from `/Users/janic/Developer/react-native/RNTester/js/[email protected]`: Module `missing-asset-registry-path` does not exist in the Haste module map ``` - The middlewares were not added to the metro server. This causes some packager server features to fail. The one I noticed is that the /status endpoint didn't exist anymore which causes CI to fail and also Android to not load the bundle from the packager initially. The remote debugging feature was also broken. Pull Request resolved: facebook/react-native#20162 Differential Revision: D8867610 Pulled By: hramos fbshipit-source-id: 8a08b7f3175692ab6ee73c0a7c25075091ae4792
This fixes some regressions with local-cli introduced in c4a66a8. Fixes #20008.
assetRegistryPath
which caused the following error when loading the bundle:Test Plan:
Release Notes:
Marking this as internal since the regression hasn't made it to a release.
[INTERNAL] [BUGFIX] [runServer.js] - Fix local-cli assetRegistryPath and middlewares