Allow live reload even on errors. #1549
Conversation
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
facebook-github-bot
added
the
CLA Signed
|
I like this idea a lot, cc @tadeuzagallo for review |
|
|
||
| [[NSNotificationCenter defaultCenter] postNotificationName:RCTJavaScriptDidLoadNotification | ||
| object:_parentBridge | ||
| userInfo:@{ @"bridge": self }]; |
There was a problem hiding this comment.
I don't think we need it here. If there was an error, there's no JavaScript running, so we don't need the display link notifications. We also can't send a notification that JavaScript has been loaded, since it wasn't.
|
Thanks for doing it. LiveReload actually still works when there's an error, the redbox is just not dismissed (to verify that you may introduce a bug, dismiss the redbox first, and then fix the bug. It should reload as expected) so adding just line that dismisses the redbox should be fine. |
tadeuzagallo
added
GH Review: needs-revision
and removed
GH Review: review-needed
labels
|
Your absolutely right about the display link not being required. But I'm not so sure about the RCTJavaScriptDidLoadNotification Notification not being required as well. Without it logic errors will auto reload just fine, but syntax errors will not. I'll do some more digging and find out why. |
facebook-github-bot
added
GH Review: review-needed
and removed
GH Review: needs-revision
labels
|
My efforts to move away from RCTJavaScriptDidLoadNotification have so far been futile. I still can't get syntax errors to reload without posting the Notification and jsLoaded: is tightly coupled to RTCDevMenu so I can't easily replicate it's behavior. I really don't like the look of posting RCTJavaScriptDidFailToLoadNotification and then RCTJavaScriptDidLoadNotification. I mean technically the documentation for RCTJavaScriptDidLoadNotification is "This notification fires when the bridge has finished loading." and technically failure is a finished state. :/ I've very unfamiliar with the code base, in fact I just laid my eyes upon it about 12 hours ago, so if I'm missing something please push me in the right direction and I'll be glad to fix it. |
|
The other thing I can think of to fix this problem would be to call jsLoaded: in RTCDevMenu when RCTJavaScriptDidFailToLoadNotification is posted. As jsLoaded: is what handles the live reload. @tadeuzagallo would you prefer I keep the new notification: RCTJavaScriptDidLoadNotification or call jsLoaded: in RTCDevMenu when RCTJavaScriptDidFailToLoadNotification is posted? |
|
@RubenSandwich it's better to listen to
|
tadeuzagallo
added
GH Review: needs-revision
and removed
GH Review: review-needed
labels
facebook-github-bot
added
GH Review: review-needed
and removed
GH Review: needs-revision
labels
|
@facebook-github-bot import |
Live reload is disabled when an error has occurred. This requires the developer to fix the error and then switch to the simulator to reload the device manually; impacting developer flow and increasing alt tabbing. This pull request fixes that by allowing live reload to work even on errors.
This fixes issue: #1343.