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

support multiple livesockets #3564

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

gfrancischelli
Copy link
Contributor

@gfrancischelli gfrancischelli commented Dec 9, 2024

What

Add rootViewSelector option to liveSocket constructor to scope which HTML nodes this liveSocket should connect.

Why

To allow multiple liveSockets in the same page, each pointing to a different domain. The motivation is enabling a microfrontend-like architecture, making it possible for different teams working on different components to have complete deployability isolation.

@gfrancischelli gfrancischelli force-pushed the support-multiple-livesockets branch 2 times, most recently from 85eb3f0 to 47f81f6 Compare December 9, 2024 10:03
Add `rootViewSelector` option to liveSocket constructor so different
liveSockets can target different liveview HTML nodes in the page.
@gfrancischelli gfrancischelli force-pushed the support-multiple-livesockets branch from 47f81f6 to ab8e7f0 Compare December 9, 2024 11:13
@@ -366,9 +369,13 @@ export default class LiveSocket {
}
}

viewSelector(){
return `${PHX_VIEW_SELECTOR}${this.rootViewSelector || ""}`
Copy link
Member

Choose a reason for hiding this comment

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

Should this be inversed, so people could do #foo-baras the root view selector if they want to? In such cases, should they end it with a space?

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 was thinking it would simplify things if we just ignore the PHX_VIEW_SELECTOR and only use the user-defined selector?

The initial idea behind composing both together was because I didn't notice we could customise the container of the LiveView. So my initial implementation had something like: :has(.some-class-inside-the-lv-template).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any reason not to just replace PHX_VIEW_SELECTOR completely with a user defined selector?

new LiveSocket("/live", Socket, {params: {viewSelector: ".my-selector"}})

We can document users need to customise the LiveView :container in that case. It can be done globally in use MyAppWeb, :liveview or easily extend to accept as param use MyAppWeb.LiveView, :liveview, app: :something 🤔

Copy link
Member

@josevalim josevalim Dec 10, 2024

Choose a reason for hiding this comment

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

I think that would be my vote but we need to tell people they will want to check for [data-phx-session] unless their selector points directly to the LiveView container.

@gfrancischelli gfrancischelli force-pushed the support-multiple-livesockets branch from 1cf5648 to 2f2e4de Compare December 9, 2024 12:48
@gfrancischelli gfrancischelli force-pushed the support-multiple-livesockets branch from 2f2e4de to 9f7150f Compare December 9, 2024 12:49
@gfrancischelli gfrancischelli marked this pull request as ready for review December 9, 2024 12:49
@victor23k
Copy link

victor23k commented Dec 10, 2024

Hi! I was actually starting to work on the same issue, though the motivation in our case is to embed LiveView apps in multiple third party sites. This PR solves the problem of sending events to all connected sockets. We also identified a couple more nice to have features:

  • Dispatch event on socket disconnection in the client side. This would allow doing an operation like hiding the rootViewElement.
  • Set an element id prefix per LiveView to ensure unique element ids.
    Example: the <.flash_group> component id will clash with other LiveViews that also use this component with the default id.

@gfrancischelli This is great work! Do you share the need to have any of these?

@josevalim Do you think these proposals make sense? Would you be open to PRs?

@SteffenDE
Copy link
Collaborator

@victor23k my gut feeling is that embedding LiveView apps would be better handled using iframes, having only the minimal code on the third party's side for gluing things together.
If you directly embed into a third-party's page, you're always working in an environment you don't fully control, duplicate IDs being a good example. Those also would not be a problem when using iframes.

@gfrancischelli
Copy link
Contributor Author

@victor23k thanks ❤️! I'm very happy to see there's more people needing this.

My use cases are:

  • Microfrontend architecture without the hassle of iframes
  • Incremental migration of legacy JavaScript web apps

Dispatch event on socket disconnection in the client side.

I'm not sure if I get it, and I don't have this use case (for now), but you can use this callback liveSocket.socket.onClose?

Set an element id prefix per LiveView to ensure unique element ids.

We didn't face collisions like this yet 🤔

@gfrancischelli
Copy link
Contributor Author

@SteffenDE For the microfrontend use case, we are trying to avoid an iFrame based solution with this.

Anyway, thank you for the great work in testing the JS part of LiveView ❤️ Do you have suggestions what else should I look to test in this PR?

@SteffenDE
Copy link
Collaborator

SteffenDE commented Dec 10, 2024

@gfrancischelli I'm not criticising your microfrontend approach. I think it's fine to do this when you are in control of the overall page, I'm just not sure if I would do this on third party sites.

Thank you for the kind words! I didn't have the time to look into your changes in detail, but I'll come back to you in the next couple of days :)

@victor23k
Copy link

@SteffenDE thanks for your suggestion! will look into that.
@gfrancischelli I'll stop hijacking your PR comments now 😅, also thanks for the tip on the callback

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

Successfully merging this pull request may close these issues.

4 participants