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

Ignore unsupported once option #29

Merged
merged 5 commits into from
May 13, 2022
Merged

Ignore unsupported once option #29

merged 5 commits into from
May 13, 2022

Conversation

cheap-glitch
Copy link
Contributor

Follow-up of #28 (comment)

Copy link
Contributor Author

@cheap-glitch cheap-glitch left a comment

Choose a reason for hiding this comment

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

Should this PR also change the accepted options types? E.g.

options?: boolean | Omit<AddEventListenerOptions, 'once'>

(But then TypeScript does complain about the runtime check.)

Copy link
Owner

@fregante fregante left a comment

Choose a reason for hiding this comment

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

Thank you!

index.ts Outdated Show resolved Hide resolved
index.ts Outdated
@@ -93,7 +94,7 @@ function delegate<
selector: Selector,
type: TEventType,
callback: delegate.EventHandler<GlobalEventHandlersEventMap[TEventType], TElement>,
options?: boolean | AddEventListenerOptions
options?: boolean | Except<AddEventListenerOptions, 'once'>
Copy link
Owner

Choose a reason for hiding this comment

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

I think we can skip the extra dependency here, especially because this is not a devDependency. This type appears in the generated index.d.ts file and will fail when imported by third parties.

Suggested change
options?: boolean | Except<AddEventListenerOptions, 'once'>
options?: boolean | Omit<AddEventListenerOptions, 'once'>

Omit is only bad because you can use it as Omit<OtherType, 'mistyped-ooonce'> and it doesn't fail. This isn't really a problem here.

Copy link
Owner

Choose a reason for hiding this comment

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

Fixing…

@fregante
Copy link
Owner

Dammit 🥲 I’ll fix it

@fregante fregante merged commit 0081742 into fregante:main May 13, 2022
@fregante
Copy link
Owner

3.0.1 published

@cheap-glitch cheap-glitch deleted the drop-once branch May 13, 2022 16:11
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.

2 participants