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

feat: delegate action events #11435

Closed
wants to merge 1 commit into from
Closed

Conversation

dummdidumm
Copy link
Member

This is a rough proposal to patch dom instances when they are passed to actions so that addEventListener delegates events. This ensures that people adding listeners imperatively will still have the event order preserved.

The code is a bit duplicative right now and it's missing cleanup/tests/logic to not patch twice, wanted to validate the idea first / get some opinions on this before proceeding.

We could also think about adding the same logic to bind:this.

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • Prefix your PR title with feat:, fix:, chore:, or docs:.
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests and linting

  • Run the tests with pnpm test and lint the project with pnpm lint

Patches dom instances when they are passed to actions so that addEventListener delegates events. This ensures that people adding listeners imperatively will still have the event order preserved.
Copy link

changeset-bot bot commented May 2, 2024

⚠️ No Changeset found

Latest commit: b8c32f5

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@trueadm
Copy link
Contributor

trueadm commented May 3, 2024

Can't we just re-use the logic in local event handlers where we run the delegated workflow? Surely it's the same thing as here?

@dummdidumm
Copy link
Member Author

Right now it's always just a single delegated listener per event because we don't allow multiple listeners per event type. But with actions you now can have potentially many of them, which complicates the whole thing. We could always use an array to simplify, not sure what implications that has on perf/memory

@Rich-Harris
Copy link
Member

This does all make me a bit nervous. Have we tried the suggestion in #11516 (comment) of adding event listeners in an effect (or rather in a microtask — we don't need the overhead of actually creating an effect, we just need things to happen in the right order)? If it's beneficial, we could presumably even save that behaviour for when an element has both events and actions.

@dummdidumm
Copy link
Member Author

We decided to not pursue this. Instead, if the need arises, we can add import { addEventListener } from 'svelte/events' which does the delegation.

@dummdidumm dummdidumm closed this May 16, 2024
@dummdidumm dummdidumm deleted the delegate-action-listeners branch May 16, 2024 19:12
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

3 participants