-
Notifications
You must be signed in to change notification settings - Fork 33
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
Bring back ability to reveal preview when stuck on waiting #696
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
this.updateProjectStateForDevice(deviceInfo, { previewURL: url }); | ||
}, | ||
}); | ||
this.updateProjectStateForDevice(this.projectState.selectedDevice!, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is called twice with previewURL
, some explanation why is that could be useful here (I assume that ideally, the second call would just set the status to "running"
, since the previewURL
has been already set at this point but I don't know the codebase).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, couldn't the second one update only "status"? then the previewURL
local variable is redundant too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The second call doesn't change anything, but relying on the fact this callback gets called every time and it gets called before the method exists is a hidden assumption I'd prefer to avoid for the cost of updating the URL twice
In #475 we accidently broke the functionality that'd allow users to tap on "waiting for app to load" text in order to reveal the device preview.
This option was pretty handy specifically to troubleshoot issues when app gets stuck because of expo devclient onboarding dialog.
This PR brings back the callback-based code that got removed in #475 with small adjustments as the underlying implementation from deviceSession got split into two separate methods in the meantime.
How Has This Been Tested: