Skip to content

Allow both HMR clients and live reloading clients #6837

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Allow both HMR clients and live reloading clients #6837

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion React/Modules/RCTDevMenu.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -653,7 +653,9 @@ - (void)checkForUpdates
if (strongSelf && strongSelf->_liveReloadEnabled) {
NSHTTPURLResponse *HTTPResponse = (NSHTTPURLResponse *)response;
if (!error && HTTPResponse.statusCode == 205) {
[strongSelf reload];
if (!strongSelf->_hotLoadingEnabled) {
[strongSelf reload];
}
} else {
strongSelf->_updateTask = nil;
[strongSelf checkForUpdates];
Expand Down
7 changes: 6 additions & 1 deletion ReactAndroid/src/main/java/com/facebook/react/devsupport/DevServerHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,7 +320,12 @@ public void startPollingOnChangeEndpoint(

private void handleOnChangePollingResponse(boolean didServerContentChanged) {
if (mOnChangePollingEnabled) {
if (didServerContentChanged) {
// before this commit, the package server doesn't inform changes to live reloading
// clients when one more HMR clients connected, this commit allow server informs
// both live reloading clients and HMR clients.
// when HMR enabled, live reload subscription doesnt't disconnect, so need disable
// live reloading.
if (didServerContentChanged && !mSettings.isHotModuleReplacementEnabled()) {
UiThreadUtil.runOnUiThread(new Runnable() {
@Override
public void run() {
Expand Down
3 changes: 2 additions & 1 deletion packager/react-packager/src/Server/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,7 +291,8 @@ class Server {
// Clear cached bundles in case user reloads
this._clearBundles();
this._hmrFileChangeListener(absPath, this._bundler.stat(absPath));
return;
// when HMR clients connected, also allow live reloading clients
// return;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the single required change to make this work but I'm not sure what would be the best way to support this feature without affecting HMR performance.

The problem is that inside of _hmrFileChangeListener lots of things happen asynchronously so if you remove the return those async operations will compete for CPU and IO with the Live Reload changes. For Live Reload it doesn't matter if the change takes a couple more of milliseconds but for HMR it does.

Even if we turn _hmrFileChangeListener into a function that returns a promise and wait to execute the other listeners, we'll end up slowing the HMR codepath because the code is not injected on the client until the sourcemaps for the new file are requested and returned (see

react-native/Libraries/Utilities/HMRClient.js

Lines 112 to 135 in 235b16d

require('SourceMapsCache').fetch({
text: code,
url: `http://${serverHost}${sourceURLs[i]}`,
sourceMappingURL: sourceMappingURLs[i],
});
// on JSC we need to inject from native for sourcemaps to work
// (Safari doesn't support `sourceMappingURL` nor any variant when
// evaluating code) but on Chrome we can simply use eval
const injectFunction = typeof global.nativeInjectHMRUpdate === 'function'
? global.nativeInjectHMRUpdate
: eval;
code = [
`__accept(`,
`${id},`,
`function(global,require,module,exports){`,
`${code}`,
'\n},',
`${JSON.stringify(inverseDependencies)}`,
`);`,
].join('');
injectFunction(code, sourceURLs[i]);
)

For this change to not affect performance we'll need to also include the sourcemaps on the original request response.

cc @satya164

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the single required change to make this work but I'm not sure what would be the best way to support this feature without affecting HMR performance.

The changes of other two files(RCTDevMenu.m and DevServerHelper.java) avoid the clients process both onchange and HMR, because clients can enable live reloading and hot reloading.

Thanks for your point, I tested competition, HMR first and Live Reload first, every are not good, just worked.

Is it possible to bundle once for Live Reload and HMR? or more common works to reduce bundle time? I can't handle this topic, I don't know details of how packager/bundler works yet.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cpunion thanks for answering so quickly and for contributing!

Is it possible to bundle once for Live Reload and HMR? or more common works to reduce bundle time? I can't handle this topic, I don't know details of how packager/bundler works yet.

Unfortunately no. We reuse much of the intermediate stuff though, but the bundle itself cannot be share as the Live Reload one is a full bundle whereas the HMR one contains only the the code of the modules that have changed + the new modules if any.

I'm not sure if it worth supporting this use case, feel like it's an edge case but I might be wrong. If it's an edge case we need to make sure it doesn't make the HMR experience worse and that the additional complexity that it adds worth it.

I think it would be very useful to create a product pains and ask around if this is something the community would like the support for :). If there's broad interest we could work on improving this pull request to avoid a hit on performance. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@martinbigio Thanks for your explain clearly. I think if HMR works fine and can replace Live Reload, we no longer need Live Reload. Yes, it seems an edge case.

Thanks again.

}

Promise.all(
Expand Down