-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Make LiveDevMultiBrowser's Launcher instance configurable via public set Launcher() method #10558
Conversation
…setLauncher() method
@humphd, looks good to me having this mechanism to dynamically set a different Just curious, have you consider some way of static configuration also for your use case? I mean, something like having a pref that allows setting the |
OK, great. Using this patch (which I landed on our brackets fork), we've just today been able to get Live Dev working in the browser with an iframe and postMessage--it's awesome :) It would be great if you'd take this patch upstream so we don't have to modify brackets at all to make it work. Regarding your other question, I've been wondering about that too. I don't love the idea of doing it with prefs, since that still forces changes in brackets proper, and isn't something you can do from an extension. I like the model you have with the live dev server manager, where you can register a server with a priority. In an ideal world, it would be great if an extension could somehow provide the transport, launcher, and maybe a few other internal implementations for Live Dev, and have Live Dev's I had the same issue with swapping out my in-browser filesystem (https://github.com/filerjs/filer) for the appshell's impementation, which requires me to change the brackets source internally. It would be great if there was a way for extensions to override more of the internal code, but that's a larger issue, and I don't want to block this bug on figuring that out if possible. |
Sorry I haven't been able to get to this for a little while. I'll review and merge this code. We'd want to have as little diffs as possible with the mainstream Brackets. As for static configuration, we'll consider it as the next step (this one may actually help it). |
*/ | ||
function setLauncher(launcher) { | ||
if (!(launcher && launcher.launch)) { | ||
throw new Error("LiveDevMultiBrowser.setLauncher(): launcher must have `launch` method"); |
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.
We'd normally log an error instead of throwing it. However, it may still make sense to have an error to be able to trace back the origin of the call. Like so (note the commas, not concatenation):
console.log("Invalid launcher object: ", launcher, new Error("LiveDevMultiBrowser.setLauncher()");
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.
Well, and return
from the if
, of course...
@humphd, looks good, only one minor comment. |
Thanks, fixed. |
*/ | ||
function setLauncher(launcher) { | ||
if (!(launcher && launcher.launch)) { | ||
console.log("Invalid launcher object: ", launcher, new Error("LiveDevMultiBrowser.setLauncher()"); |
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.
@humphd, this has a syntax error. :)
aa40d73
to
7bc69e4
Compare
Ugh. Fixed. |
@busykai ping? |
Looks good! Merging. Sorry for the delay. |
Make LiveDevMultiBrowser's Launcher instance configurable via public set Launcher() method
Great, thanks! |
We're working on using the new LiveDevMultiBrowser code to allow an in-browser Brackets to use an iframe-based browser and postMessage-based protocol, see https://github.com/humphd/brackets/issues/27. One of the snags we've hit is that we need a way to override how the default browser is launched, and the
Launcher
is currently not configurable.This patch follows the same pattern currently in place for swapping the protocol's transport (which we also make use of in order to use postMessage to/from the iframe), and adds a
setLauncher()
method. If you'd consider this API change, we'd be able to get our own iframe-based launcher hooked into live dev.cc @busykai, @sebaslv, @sedge