Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Live Preview MultiBrowser fails to connect in this case #10374

Closed
redmunds opened this issue Jan 14, 2015 · 8 comments · Fixed by #10453
Closed

Live Preview MultiBrowser fails to connect in this case #10374

redmunds opened this issue Jan 14, 2015 · 8 comments · Fixed by #10453

Comments

@redmunds
Copy link
Contributor

Live Preview MultiBrowser fails to connect in this case. More details in these other comments in PR #10285

  1. Start Bracket s on a system with a local server setup and running
  2. Switch to a site with files on the local server
  3. I originally hit this by setting base url to be http:/localhost/, but this is not required
  4. Switch to LiveDev MultiBrowser
  5. Start Live Dev

Results
Live Dev never completely connects. It stays in Connecting state.

sebaslv added a commit to sebaslv/brackets that referenced this issue Jan 23, 2015
Fix for adobe#10374. LiveDevMultiBrowser can't load when a local server is configured.
Servers registered by LiveDevelopment are now being cleaned-up when closing the
session.
@peterflynn
Copy link
Member

@redmunds @sebaslv Isn't the root problem here that Multi-Browser doesn't support custom servers? (also alluded to in #10217)

Is the idea that Multi-Browser should just ignore the base URL and load the page statically as if no URL was set? At a glance that seems like the sort of behavior PR #10453 is shooting for.

Getting it to actually respect a custom server is a much more complex undertaking that I don't think we have a good answer for yet, right? For the same reason that we can't do HTML live updating on user servers with the 'classic' Live Preview impl (see Trello card): we can't inject content into the HTML page without having some of our code in the server side.

@sebaslv
Copy link
Contributor

sebaslv commented Mar 5, 2015

Yes, the root cause is that MB doesn't support custom servers but, what makes this issue being manifested is that servers registered by LiveDevelopment are not being cleaned-up when switching to MB which makes LiveDevMultiBrowser not able to get StaticServer from LiveDevServerManager. It returns UserServerbecause of priority if URL was set before. StaticManager is needed by MB to make instrumentation possible.

This PR proposes to solve this issue by extending LiveDevServerManager to let LiveDevelopment register/remove the servers when switching btw implementations so only StaticServer keeps registered when running MB.

It follows @redmunds's recommendation from PR #10285.

@peterflynn
Copy link
Member

I wonder what the "correct" behavior is when the user has a custom server URL set, though. Silently serving up the page statically instead seems confusing -- maybe we should show a warning dialog the first time (per project) that Multi-Browser Live Preview is launched while a server URL (that it will ignore) is in effect.

@peterflynn
Copy link
Member

Also -- I take back one part of what I said in my first comment. Solving this Multi-Browser limitation could be a good deal simpler than getting Live HTML to work with a custom server. Since all we need to do for Multi-Browser is inject a single <script> tag, at least two simpler options would work:

  • Interject our own proxy server in front of the user's custom server, and have the browser go to our URL. Our proxy injects the tag on the fly. (Thanks to @njx for pointing this out). This could still break some users' projects where the domain name the browser sees is important (e.g. pages making CORS requests), but it would work for most people.
  • Provide users a bookmarklet that they can click on in their browser to add the instrumentation (Weinre does this). We miss the initial page-load related events, but I don't think we currently need them, do we? (The bigger downside is it's just more of a pain for users, and more of a pain for us to build a UI & docs that explain how to do it).

@busykai
Copy link
Contributor

busykai commented Apr 13, 2015

@peterflynn, @sebaslv I think we're dealing with two issues here:

  1. Confusion and actual errors (as the one described in this issue) due to the fact that multi-browser does not support UserServer.
  2. The fact that multi-browser implementation does not support UserServer configuration. @peterflynn's later comments outline some strategies on how it can be fixed with lesser effort.

I think PR #10453 is an adequate fix for 1). Item 2) is a separate issue and needs to be addressed as such.

So I'm going to review and merge #10453 and start the work on #10217 (which is medium priority, but I haven't got a chance to advance on it at all)...

@redmunds
Copy link
Contributor Author

@busykai Looks like this was auto-closed. Should it be re-opened for the other half?

@busykai
Copy link
Contributor

busykai commented May 14, 2015

@redmunds, we have #10217 to track the user server case (this is the part that is not fixed, right?).

@redmunds
Copy link
Contributor Author

@busykai Ah, yes. Nevermind :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants
@redmunds @peterflynn @busykai @sebaslv and others