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

Set host of development server for setupDevtools #15547

Closed
wants to merge 4 commits into from

Conversation

jhen0409
Copy link
Contributor

@jhen0409 jhen0409 commented Aug 18, 2017

Related to #15126, and this would be useful for use React DevTools on real device without modify setupDevtools.js.

In Android emulator, the host of SourceCode.scriptURL is same with PlatformConstants.ServerHost so we can just replace it.

Test Plan

  • Tested on iOS device with react-devtools package.
  • Tested on Android emulator, the getDevServer module got the correctly hostname so that don't need adb reverse.

Release Notes

[ENHANCEMENT] [setupDevtools] Set host of development server for setupDevtools

@facebook-github-bot facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Aug 18, 2017
@jhen0409 jhen0409 force-pushed the setup-devtools-host branch from 93db9be to 98a0aba Compare August 18, 2017 10:17
@macrozone
Copy link

This would be awesome to have. In particular with https://github.com/jhen0409/react-native-debugger. Any update on this?

@jhen0409 jhen0409 force-pushed the setup-devtools-host branch from ad3d582 to 54511f3 Compare October 5, 2017 10:11
@jhen0409
Copy link
Contributor Author

/cc @javache

@facebook-github-bot
Copy link
Contributor

@jhen0409 I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

@gaearon
Copy link
Collaborator

gaearon commented Dec 12, 2017

@javache is not working on RN anymore so we'll need to find somebody else to review this.

@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 Dec 12, 2017
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.

@javache is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@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 Dec 13, 2017
@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.

@pull-bot
Copy link

Warnings
⚠️

📋 Release Notes - This PR appears to be missing Release Notes.

@facebook-github-bot label Needs more information

Generated by 🚫 dangerJS

@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 Dec 14, 2017
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.

@javache is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@pull-bot
Copy link

Warnings
⚠️

📋 Release Notes - This PR may have incorrectly formatted Release Notes.

@facebook-github-bot label Needs more information

Generated by 🚫 dangerJS

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.

@javache is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

mathiasbynens pushed a commit to mathiasbynens/react-native that referenced this pull request Jan 13, 2018
Summary:
Related to facebook#15126, and this would be useful for use React DevTools on real device without modify `setupDevtools.js`.

In Android emulator, the host of `SourceCode.scriptURL` is same with `PlatformConstants.ServerHost` so we can just replace it.

* Tested on iOS device with [react-devtools](https://github.com/facebook/react-devtools/tree/master/packages/react-devtools) package.
* Tested on Android emulator, the `getDevServer` module got the correctly hostname so that don't need `adb reverse`.

[ENHANCEMENT] [setupDevtools] Set host of development server for setupDevtools
Closes facebook#15547

Differential Revision: D6544980

Pulled By: javache

fbshipit-source-id: a286874bcef0501c5d2e0be2251d58c236a5534a
@vladkosinov
Copy link

Can someone merge it?
Seems like there is no way to use react-devtools for real devices without changing host manually in Libraries/Core/Devtools/setupDevtools.js :(

@gaearon

@macrozone
Copy link

It‘s extremly sad that this pr has been closed :(

This is a very useful change, developing on a feal device is not unusual and sometimes the only way

@jhen0409
Copy link
Contributor Author

Actually this change have been landed in master, it should be released as RN 0.53.

Thanks @javache come back to deal with this. 😃

@jhen0409 jhen0409 deleted the setup-devtools-host branch January 25, 2018 12:43
@gaearon
Copy link
Collaborator

gaearon commented Feb 6, 2018

The PR appears as closed because RN is set up to sync to our internal repository. So all PRs are “closed” on GitHub when they’re imported, but the commits appear on master.

@hramos hramos added the Merged This PR has been merged. label Mar 8, 2019
@react-native-bot react-native-bot removed the Import Started This pull request has been imported. This does not imply the PR has been approved. label Mar 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants