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

[HMR] Multiple HMR clients #6179

Closed
wants to merge 2 commits into from

Conversation

JohnyDays
Copy link
Contributor

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
Copy link
Contributor

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

@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 Feb 26, 2016
@skevy
Copy link
Contributor

skevy commented Feb 26, 2016

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
Copy link
Contributor

@JohnyDays updated the pull request.

@JohnyDays
Copy link
Contributor Author

@skevy Yeah sure, it's done 👍

@JohnyDays
Copy link
Contributor Author

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

@martinbigio
Copy link
Contributor

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
Copy link
Contributor

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

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

let platformCache = {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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 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 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

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:

  1. State (the cache) will be collocated with behavior (processFileChange) which will make this code a bit more object oriented.
  2. You'll have to deal with the cache for the client instead of with a cache for multiple clients.
  3. Sending updates will be per client so you won't have to maintain an object with the updates you have to send.
  4. Error handling per client will be easy to implement.
  5. All this code will become much easier to test in isolation.

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.

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
Copy link
Contributor

The demo you posted is amazing man!

@JohnyDays
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@martinbigio
Copy link
Contributor

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

@martinbigio
Copy link
Contributor

@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
Copy link
Contributor Author

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
Copy link
Contributor

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

@JohnyDays
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

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

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link

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
Copy link

ghost commented Apr 11, 2016

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

@JohnyDays
Copy link
Contributor Author

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

@ptomasroos
Copy link
Contributor

Great work! Looking forward to it!

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

Choose a reason for hiding this comment

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

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

@JohnyDays
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@JohnyDays JohnyDays closed this May 9, 2016
@asteingass
Copy link

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
Copy link

dk0r commented Jul 21, 2017

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

@iBasit
Copy link

iBasit commented Dec 9, 2017

no links?

@asteingass
Copy link

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

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet