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

allows addEventListener to use handleEvent syntax while re-using the object for options #341

Merged
merged 13 commits into from
Sep 9, 2024

Conversation

titoBouzout
Copy link
Contributor

@titoBouzout titoBouzout commented Aug 17, 2024

This solves in a simple way the common need of wanting to use passive/once/capture events.

addEventListener has an "obscure" feature of allowing the listener to be in an object with key handleEvent

This PR allows this syntax:

<div on:click={ {
  handleEvent(e) {
    console.log("clicked", e)
  },
  once:true
}/>

I haven't tested this but it should work https://playground.solidjs.com/anonymous/df01ffcb-e7dc-4a36-b80d-51a398c02689

This is what pota does https://github.com/potahtml/pota/blob/master/src/lib/reactive.js#L1372

@titoBouzout
Copy link
Contributor Author

Probably oncapture: can be removed here, but that would be a breaking change https://github.com/search?q=repo%3Aryansolid%2Fdom-expressions%20oncapture&type=code

@lxsmnsyc
Copy link
Collaborator

This is actually an interesting feature to put in.

@ryansolid
Copy link
Owner

Yeah I like the implications of this. Not needing oncapture: etc and allowing full config of native events seems a real win. I might be releasing one more minor of Solid 1.x and I'd like to see this get in.

@davedbase
Copy link

@titoBouzout I hope we don't forget to add this to the docs. :)

@titoBouzout
Copy link
Contributor Author

@davedbase already on my mind, I will look if typings need updates and how to add tests so gets ready for merging, stay tuned

@titoBouzout titoBouzout marked this pull request as draft September 3, 2024 12:41
@titoBouzout
Copy link
Contributor Author

titoBouzout commented Sep 4, 2024

ok if I am not missing anything I think this is ready for review

  • updated types adding a CustomEventHandlerUnion which accepts a regular function or the object form
  • it will use dom-expression addEventListener instead of the browser one, as it has the logic for checking if options it's an object. This has the nice side effect that output when minified should become smaller as node.addEventListener(...) will become $addEventListener(node, ...)
  • added compilation test for expected output
  • added jsdom test for actually checking if the options object is working as expected
  • marked interface CustomCaptureEvents as deprecated

Considerations:

  • is there any code related to hydration that needs an update?
  • please double check my typings, can the addEventListener function signature types be improved to use the ones defined on client.d.ts?

should close:
solidjs/solid#2210
solidjs/solid#1939
solidjs/solid#1786

Thanks!

@titoBouzout titoBouzout marked this pull request as ready for review September 4, 2024 12:52
@titoBouzout
Copy link
Contributor Author

@ryansolid pinging as it was noted the desired to include it on next release

@titoBouzout
Copy link
Contributor Author

docs update at solidjs/solid-docs#866

@ryansolid ryansolid changed the base branch from main to next September 9, 2024 20:59
@ryansolid ryansolid merged commit 0bee0fe into ryansolid:next Sep 9, 2024
2 checks passed
ryansolid pushed a commit that referenced this pull request Sep 9, 2024
…the object for `options` (#341)

* allow `addEventListener` to use `handleEvent` syntax while using the object form for options

* add type to client.d.ts

* add type to lit-dom-expressions/src/index.ts

* add to `assignProp`

* add to `removeEventListener` on client.js

* Update types

* Update types

* formatting

* add `EventHandlerWithOptions` type

* use  dom-expressions `addEventListener` instead of the browser one for custom events. Move `oncapture` to its own conditional

* add test for expected compile output

* add test using jsdom to assert a handler with `once` only runs once
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