[HMR] Multiple HMR clients #6179

JohnyDays · 2016-02-26T19:17:35Z

Multiple HMR clients

This pull request adds support for multiple HMR clients 🎈 🎂 🎈 (as per discussed in #5338 and e018aa3)

How it works

Dependency caches are being kept per unique bundle/platform combination in a structure such as

{
  ios: {
    'path/to/index.ios.bundle': [/* this bundle's dependencies */],
  },
  android: {
    'path/to/index.android.bundle': [/* this bundle's dependencies */],
  },
}

Upon receiving a connection request, it's added to a similar structure per bundle

{ 
  [bundle]: [/*connected clients for this bundle*/] 
}

The HMR bundle changes are then forwarded to each device when their bundle's/platform dependencies are invalid/changed.

EDIT: Quick demo 😄 https://www.youtube.com/watch?v=etk3Bb7z1eQ

Test Plan

I did some manual tests using 2 emulators and 2 real devices, all connected at the same time, hot reloading successfully! (1 android emulator, 1 android devices, 1 iOS emulator and 1 iOS device).

Is there any way I can automate this? Not familiar with the testing practices for similar features on the project.

@martinbigio You seem like the guy to ask for code review 😄 (EDIT: Oh that's an helpful bot)

…orms and apps, to receive HMR updates in parallel

facebook-github-bot · 2016-02-26T19:18:20Z

By analyzing the blame information on this pull request, we identified @martinbigio, @skevy and @davidaurelio to be potential reviewers.

skevy · 2016-02-26T19:26:42Z

This looks pretty good @JohnyDays!

One nit: can you remove the new lines after each function definition, such as here: https://github.com/facebook/react-native/pull/6179/files#diff-927b035f11215f6000c2e633e88d8991R29?

I can't test right now...but will do so later. This will be a helpful feature.

facebook-github-bot · 2016-02-26T19:37:14Z

@JohnyDays updated the pull request.

JohnyDays · 2016-02-26T19:37:43Z

@skevy Yeah sure, it's done 👍

JohnyDays · 2016-02-26T23:25:15Z

Added a quick demo showing it off, still got those ugly warnings tho 😢 it'd be cool if that was fixed before release.

martinbigio · 2016-02-27T20:02:40Z

Oops commented on the other thread by mistake. Copying post here.

Wow, this indeed will be helpful for a lot of people!.

I recently added support for redux stores and modules that export pure functions and modified this file (436db67). Most likely you'll have to rebase.

martinbigio · 2016-02-27T20:03:59Z

Looks like you're getting the yellow box only on Android. No errors on the packager server, right?

martinbigio · 2016-02-27T20:07:04Z

local-cli/server/util/attachHMRServer.js

+  let clients = [];
+  let clientsPerBundleEntry = {};
+
+  let platformCache = {


Some of the logic on the file would become simpler if we remove one level here (i.e.: concatenate the platform with the bundleEntry). Maybe we could turn client into a class and define a hash function based on the bundleEntry and the platform. After all you want a different cache for each client.

I thought about merging the bundle entry and the platform, but I was unsure of whether the platform was relevant for something beyond the bundle entry, in another part of the packager.

Also, do we really want a cache for each client? The client's only identifying features are the bundleEntry and it's platform, so if we have a cache for each of those, it should be enough right? And this way, for the most common case (Developing one application, on multiple devices) we are at most compiling 2 bundles, one for iOS, and one for android, since all the devices for the same platform share the same bundleEntry. And we still support multiple applications at the same time, since they have different bundleEntries, and as such different caches 👍

I thought about merging the bundle entry and the platform, but I was unsure of whether the platform was relevant for something beyond the bundle entry, in another part of the packager

I was suggesting doing that only on this file which is pretty isolated from the rest of the packager.

Also, do we really want a cache for each client?

On your PR we're having different caches for different combinations of bundleEntry + platform, which represents different clients except rare case on which you have both the simulator and a device connected to the Packager. We could easily make the client class support multiple web socket connections to support this use case as well. I think introducing the client class will make the code simpler because:

State (the cache) will be collocated with behavior (processFileChange) which will make this code a bit more object oriented.

You'll have to deal with the cache for the client instead of with a cache for multiple clients.

Sending updates will be per client so you won't have to maintain an object with the updates you have to send.

Error handling per client will be easy to implement.

All this code will become much easier to test in isolation.

What do you think? :)

except rare case on which you have both the simulator and a device connected to the Packager

I was imagining my own use case, which is testing on different screen sizes at the same time, by connecting an iPhone and an iPad for example, or an android phone and a tablet, or all of the above! 😄
This would effectively cut the processing time by half in that specific case.

On the other hand, I definitely agree with your approach, this should be isolated, which would lead to better error handling, and more maintainability.

I think the tradeoff in speed is more than acceptable, especially since my use case should be pretty rare, most people would only want to develop on android and iOS simultaneously, and this complexity wouldn't speed that up at all.

martinbigio · 2016-02-27T20:27:04Z

The demo you posted is amazing man!

JohnyDays · 2016-02-28T01:09:39Z

Thanks @martinbigio for the code review! I think your approach makes sense, by isolating the process per client, and then distributing just the changed filename to each client, we can have more robust per-client error handling, simpler code and easier testing, for marginal performance loss in most cases.

We can also optimize it later using some tricks, though this isolated approach does make that harder.

JohnyDays · 2016-02-28T01:18:29Z

When do you guys think HMR will go out in a stable release? I'll try to get these changes done for that.

martinbigio · 2016-02-28T23:39:45Z

We'll cut 0.22 mid/late this week. Would be awesome if we could include this PR :)

martinbigio · 2016-02-28T23:50:44Z

@JohnyDays feel free to ping me on messenger if you want (facebook.com/martin.bigio). I tend to be way more responsive than on Github :)

JohnyDays · 2016-03-01T12:07:19Z

Pretty busy week, I'll try to sneak this in. If it's not done in the meanwhile, we'll try to get it in 0.23.
@martinbigio sure, if I need some guidance I'll ask you there 😄

martinbigio · 2016-03-21T16:41:07Z

@JohnyDays any update on this? would be awesome releasing it on 0.24 :)

JohnyDays · 2016-03-21T20:57:04Z

I took some time to work on it this weekend but I didn't have the opportunity to finish it.
I'll try to take some time this week, cheers 🎸

martinbigio · 2016-03-21T20:59:19Z

Awesome! FYI I'm writing a blog post about HMR and I think I'll mention this as one of the hot features we'll soon support :)

JohnyDays · 2016-03-22T11:36:35Z

Cool! Having followed the community for a while, I'm glad I get to add another drop of awesome into the pool

solomonhawk · 2016-03-28T21:58:44Z

local-cli/server/util/attachHMRServer.js

+  if (valueOrPromise == null) {
+    return Promise.resolve(valueOrPromise)
+  }
+  else if (valueOrPromise.then && typeof valueOrPromise === "function") {


When duck-typing Promises, I'm used to seeing:

if ('then' in thing && typeof thing.then === 'function') { .. }

Should this be checking whether valueOrPromise.then is a function rather than valueOrPromise directly?

Apologies if I'm overlooking something here.

Definitely! It was a mistake. In practice if the first one is true then it rarely matters but it could be an issue for some promise-likes. This code is being rewritten in a new style so I won't fix it right now.
Thanks though!

ashleydw · 2016-03-30T20:10:42Z

Great work @JohnyDays!

Looking forward to this landing. I've modified my run-ios command to allow me to run multiple ios simulators using the command format:

react-native run-ios --simulator "iPhone 6, iPad Retina"

Demo: https://drive.google.com/open?id=0BwTfufgK1OxfUW5vYzM0WEpaVFU

Once merged, I'll PR my updated run-ios file

ghost · 2016-04-11T17:00:32Z

@martinbigio would you mind taking a look at this pull request? It's been a while since the last commit was reviewed.

JohnyDays · 2016-04-11T20:27:30Z

Yeah I wanna take a look at it, work has been chaos but hopefully I get this done soon

ptomasroos · 2016-04-12T07:42:17Z

Great work! Looking forward to it!

cpunion · 2016-04-19T02:34:45Z

local-cli/server/util/attachHMRServer.js

+    if (platform && bundleEntry) {
+      clientsPerBundleEntry[bundleEntry].map(client => {
+        if (client.platform === platform) {
+          client.ws.send(JSON.stringify(message))


I modified a similar feature for myself, sometimes send raises exceptions, so need catch it. Maybe Broken Pipe, I don't remember.

JohnyDays · 2016-05-09T15:59:58Z

I got around to doing this, should I open a new pull request? @martinbigio
The code is quite different, and I'm not sure how to change which branch the pull request is pointing to, is that even possible? Cheers

JohnyDays · 2016-05-09T16:46:49Z

You can't change the branch so I created a new PR, check #7475

asteingass · 2017-03-24T08:18:25Z

Hello everyone. It looks like this feature got implemented. How can I use it now? How can I do hot reload on 2 devices at the same time?
Thanks
Andreas

dk0r · 2017-07-21T14:14:53Z

@asteingass Would you please link where you see it was implemented?

iBasit · 2017-12-09T04:10:32Z

no links?

asteingass · 2018-01-08T09:22:08Z

Sorry guys, I just saw the PR #7475 and it's closed so I thought its done.

[HMR] Allow for multiple HMR client connections, from different platf…

982de65

…orms and apps, to receive HMR updates in parallel

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 Feb 26, 2016

Removed newlines following function definitons, and an extra log

e954ab2

martinbigio reviewed Feb 27, 2016
View reviewed changes

solomonhawk reviewed Mar 28, 2016
View reviewed changes

cpunion mentioned this pull request Apr 6, 2016

Allow both HMR clients and live reloading clients #6837

Closed

cpunion reviewed Apr 19, 2016
View reviewed changes

JohnyDays mentioned this pull request May 9, 2016

Support multiple HMR clients #7475

Closed

JohnyDays closed this May 9, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HMR] Multiple HMR clients #6179

[HMR] Multiple HMR clients #6179

JohnyDays commented Feb 26, 2016

facebook-github-bot commented Feb 26, 2016

skevy commented Feb 26, 2016

facebook-github-bot commented Feb 26, 2016

JohnyDays commented Feb 26, 2016

JohnyDays commented Feb 26, 2016

martinbigio commented Feb 27, 2016

martinbigio commented Feb 27, 2016

martinbigio Feb 27, 2016

JohnyDays Feb 27, 2016

martinbigio Feb 27, 2016

JohnyDays Feb 28, 2016

martinbigio commented Feb 27, 2016

JohnyDays commented Feb 28, 2016

JohnyDays commented Feb 28, 2016

martinbigio commented Feb 28, 2016

martinbigio commented Feb 28, 2016

JohnyDays commented Mar 1, 2016

martinbigio commented Mar 21, 2016

JohnyDays commented Mar 21, 2016

martinbigio commented Mar 21, 2016

JohnyDays commented Mar 22, 2016

solomonhawk Mar 28, 2016

JohnyDays Mar 29, 2016

ashleydw commented Mar 30, 2016

ghost commented Apr 11, 2016

JohnyDays commented Apr 11, 2016

ptomasroos commented Apr 12, 2016

cpunion Apr 19, 2016

JohnyDays commented May 9, 2016

JohnyDays commented May 9, 2016

asteingass commented Mar 24, 2017

dk0r commented Jul 21, 2017

iBasit commented Dec 9, 2017

asteingass commented Jan 8, 2018

[HMR] Multiple HMR clients #6179

[HMR] Multiple HMR clients #6179

Conversation

JohnyDays commented Feb 26, 2016

Multiple HMR clients

How it works

Test Plan

facebook-github-bot commented Feb 26, 2016

skevy commented Feb 26, 2016

facebook-github-bot commented Feb 26, 2016

JohnyDays commented Feb 26, 2016

JohnyDays commented Feb 26, 2016

martinbigio commented Feb 27, 2016

martinbigio commented Feb 27, 2016

martinbigio Feb 27, 2016

Choose a reason for hiding this comment

JohnyDays Feb 27, 2016

Choose a reason for hiding this comment

martinbigio Feb 27, 2016

Choose a reason for hiding this comment

JohnyDays Feb 28, 2016

Choose a reason for hiding this comment

martinbigio commented Feb 27, 2016

JohnyDays commented Feb 28, 2016

JohnyDays commented Feb 28, 2016

martinbigio commented Feb 28, 2016

martinbigio commented Feb 28, 2016

JohnyDays commented Mar 1, 2016

martinbigio commented Mar 21, 2016

JohnyDays commented Mar 21, 2016

martinbigio commented Mar 21, 2016

JohnyDays commented Mar 22, 2016

solomonhawk Mar 28, 2016

Choose a reason for hiding this comment

JohnyDays Mar 29, 2016

Choose a reason for hiding this comment

ashleydw commented Mar 30, 2016

ghost commented Apr 11, 2016

JohnyDays commented Apr 11, 2016

ptomasroos commented Apr 12, 2016

cpunion Apr 19, 2016

Choose a reason for hiding this comment

JohnyDays commented May 9, 2016

JohnyDays commented May 9, 2016

asteingass commented Mar 24, 2017

dk0r commented Jul 21, 2017

iBasit commented Dec 9, 2017

asteingass commented Jan 8, 2018