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

Fix local-cli assetRegistryPath and middlewares #20162

Closed
wants to merge 3 commits into from

Conversation

janicduplessis
Copy link
Contributor

@janicduplessis janicduplessis commented Jul 12, 2018

This fixes some regressions with local-cli introduced in c4a66a8. Fixes #20008.

  • 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/uie_thumb_normal@2x.png`: 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.

Test Plan:

  • Run RNTester and make sure the app loads properly.
  • Test the /status endpoint to make sure middlewares are added.
  • Test remote debugging and make sure it works as expected.

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

janicduplessis referenced this pull request Jul 12, 2018
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
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 12, 2018
@janicduplessis
Copy link
Contributor Author

CI is back to green with this :)

@janicduplessis
Copy link
Contributor Author

I found more issues with remote debugging because we don't attach the websocket proxy, looking into a fix.

@janicduplessis
Copy link
Contributor Author

Looks like hmr is not working also 🙃

@janicduplessis
Copy link
Contributor Author

A lot of config keys were on the wrong object, hmr now works.

@facebook-github-bot facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Jul 16, 2018
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a 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.

@facebook-github-bot
Copy link
Contributor

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.

@facebook-github-bot facebook-github-bot added Import Failed and removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Jul 17, 2018
@@ -55,21 +59,32 @@ async function runServer(args: Args, config: ConfigT) {
const serverInstance = await Metro.runServer({
Copy link
Contributor

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]

Copy link
Contributor

@hramos hramos left a 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.

@janicduplessis
Copy link
Contributor Author

@hramos Oh right, we ignore metro flow types in OSS so I it must be why I missed it. Working on a fix.

@janicduplessis
Copy link
Contributor Author

The watch key isn't used by metro (it overrides it to true always), not sure what the nonPersistent is for but breaking this is better than the current state of things :)

@janicduplessis
Copy link
Contributor Author

@hramos Should be good to land now

@facebook-github-bot facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Jul 17, 2018
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a 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.

@react-native-bot
Copy link
Collaborator

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 master tag, it is not yet part of a release.

@facebook facebook locked as resolved and limited conversation to collaborators Jul 18, 2018
@react-native-bot react-native-bot added the Merged This PR has been merged. label Jul 18, 2018
grabbou pushed a commit to react-native-community/cli that referenced this pull request Sep 26, 2018
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/uie_thumb_normal@2x.png`: 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
@hramos hramos removed Import Started This pull request has been imported. This does not imply the PR has been approved. Import Failed labels Feb 6, 2019
@facebook-github-bot facebook-github-bot added the Contributor A React Native contributor. label Apr 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Contributor A React Native contributor. Merged This PR has been merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CI] Test failure - D8711717 test_objc, test_end_to_end
4 participants