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

Mechanism for bubbling all events #4599

Closed
wants to merge 2 commits into from

Conversation

RedHatter
Copy link
Contributor

This PR enables the use of the on:* directive to bubble all events to the parent. See #2837. I removed the bubble utility function and instead the Component.$on function delegates to Fragment.b which connects the appropriate listeners directly or does so after the component mounts. This has an advantage over the previous approach as it doesn't require adding an event listener to the element when no bubbled events are listened to.

@RedHatter
Copy link
Contributor Author

Rebased off master.

@Conduitry Any chance this can be looked at soon? I don't believe there's anything controversial about this one.

@thes0n1x

This comment has been minimized.

@Conduitry
Copy link
Member

This is conflicting now with the work from #4860. I briefly tried to resolve it but was unable to.

@RedHatter RedHatter force-pushed the bubble-all branch 2 times, most recently from 92f1b48 to 3d6b990 Compare June 2, 2020 16:10
@RedHatter
Copy link
Contributor Author

@Conduitry Should be good now

@antony
Copy link
Member

antony commented Sep 30, 2020

I've rebased this again on the current master so it should cleanly merge now.

@Leonidaz Leonidaz mentioned this pull request Nov 4, 2020
@btakita
Copy link
Contributor

btakita commented Dec 8, 2020

Is this feature still being considered for inclusion?

@janosh
Copy link
Contributor

janosh commented Dec 29, 2020

This PR looks like it needs only a tiny bit more attention to get merged.

@Drevoed

This comment has been minimized.

@ZerdoX-x
Copy link
Contributor

ZerdoX-x commented Mar 3, 2021

I hope this will be merged soon <3

@benmccann
Copy link
Member

Some thoughts from the other maintainers: Having a dedicated bubble: directive may be preferable to the on: directive. bubble:foo could do the same thing as what a lone on:foo does now, and would become the preferred syntax. on:foo is problematic because we can't do on:foo|preventDefault to prevent the default without bubbling. on:foo|preventDefault bubble:foo would be more explicit and more useful. Obviously we can't change the behaviour of on:foo until v4, but in the meantime we may wish to avoid doubling down.

@flipkickmedia
Copy link

flipkickmedia commented Sep 27, 2021

Some thoughts from the other maintainers: Having a dedicated bubble: directive may be preferable to the on: directive. bubble:foo could do the same thing as what a lone on:foo does now, and would become the preferred syntax. on:foo is problematic because we can't do on:foo|preventDefault to prevent the default without bubbling. on:foo|preventDefault bubble:foo would be more explicit and more useful. Obviously we can't change the behaviour of on:foo until v4, but in the meantime we may wish to avoid doubling down.

Id be an advocate of providing the bindings as bubble:<xyz> but if you are allowing a parent to provide handling of the events of a child, before they are known then it would be sensible to assume you need to pass these in at runtime as some sort of config.

Would: <svelte:component this={componentInstance} bubble:config={config} /> be acceptable? Im not sure it's in the scope of this PR but this would seem a more sensible way to allow bubbling in the spirit of the concept.

@Florian-Schoenherr
Copy link

The merge-conflicts are really small, I also made a fork and fixed them, but would be silly to do a new PR for this, as it is @RedHatter's work.

@Tropix126
Copy link

Tropix126 commented Oct 7, 2021

Here is an RFC that I wrote which proposes another possible solution: sveltejs/rfcs#60

@IgorLo
Copy link

IgorLo commented Feb 26, 2022

Seems like @RedHatter has not been active on GitHub for a while
Is there some problems with the review? Seems like this PR should have been merged at least a year ago

@flipkickmedia
Copy link

It needs more work. It breaks a lot of the tests but in actual use, Ive not found a single problem using it. Maybe in SSR it presents problems. Ive since done some updates to it so once Ive revisited this (Im using this in a production system) Ill push these changes and see if I can get it to pass the tests.

@MrHBS
Copy link

MrHBS commented Mar 30, 2024

Looks like that issue was closed. Is this PR still relevant?

@dummdidumm
Copy link
Member

Closing as outdated - Svelte 5 event attributes will solve this

@dummdidumm dummdidumm closed this Mar 30, 2024
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.

None yet