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

[POC] feat: migrate to Broadcast Channel API #600

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

skjnldsv
Copy link
Contributor

@skjnldsv skjnldsv commented Apr 27, 2023

This is a first DIRTY try to support cross-tab event sharing.
Here you can see it work in action (see the favorites list in the navigation):

Peek.27-04-2023.14-42.mp4

Refs

Issues:

  • If you emit complex objects that have bindings to window, it will fail hard.
    e.g. I was emitting a files:legacy-view:initialized with this as data, and the try...catch didn't prevent it from failing. You will get a failed to execute 'postMessage' on 'BroadcastChannel'

@ChristophWurst @nickvergessen @juliushaertl (feel free to loop anyone you'd like to discuss this)

@skjnldsv skjnldsv added enhancement New feature or request help wanted Extra attention is needed labels Apr 27, 2023
@skjnldsv skjnldsv self-assigned this Apr 27, 2023
@ChristophWurst
Copy link
Contributor

This should not be the default, right? Most events will still be local to the tab. Do we need a second emit for these cross-tab events?

@skjnldsv

This comment was marked as resolved.

@nickvergessen
Copy link
Contributor

cc @ShGKme

@ChristophWurst
Copy link
Contributor

This should not be the default, right? Most events will still be local to the tab. Do we need a second emit for these cross-tab events?

Do we lose anything by not making those event global? thinking EDIT: I'm also ok making it local :)

For some events the context is local. E.g. for a sidebar opened event.

@ShGKme
Copy link

ShGKme commented Apr 27, 2023

Do we lose anything by not making those event global?

It seems that most (or just many) of events are local.

Even with global events, it may not work globally, depending on a local data. (mentioned in the PR as complex data issue).

Or the opposite. It works globally, but each tab emits the event, like

  1. Open 10 tabs
  2. Receive a notification
  3. Each tab makes emit()
  4. We have 10 notifications

Examples are very "pseudo", but it is very related on the usage.

Copy link

@ShGKme ShGKme left a comment

Choose a reason for hiding this comment

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

🚀🚀🚀

I think, using the Broadcast channel is a great option, that may improve many things in UX and performance / load.

In some cases moving to a "global" approach would probably require also a data sync or moving some modules to a Shared Worker or Web Locks API to implement Leader pattern. And this update in the event hub is a great start to change the way we work with updates in many apps, and to make it uniformly.

Looking forward for this update 🚀

Signed-off-by: John Molakvoæ <skjnldsv@protonmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants