Skip to content

fix: Incorrect onToggle event type#4615

Merged
rschristian merged 2 commits intomainfrom
fix/on-toggle-types
Dec 20, 2024
Merged

fix: Incorrect onToggle event type#4615
rschristian merged 2 commits intomainfrom
fix/on-toggle-types

Conversation

@rschristian
Copy link
Copy Markdown
Member

Closes #4614

Comment on lines -383 to -384
h('option', { value: 'foo' });
createElement('option', { value: 'foo' });
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Sorry for the diff, placed this rather wonkily a few weeks ago. Moved it out so it wasn't sat in the middle of the event tests.

@rschristian
Copy link
Copy Markdown
Member Author

rschristian commented Dec 19, 2024

Damn, ToggleEvent is newer than our TS version. I'd rather not raise the type floor

Edit: ToggleEvent was added in TS 5.2

@rschristian rschristian marked this pull request as draft December 19, 2024 19:15
Comment on lines +23 to +43
// Remove when bumping TS minimum to >5.2

/** [MDN Reference](https://developer.mozilla.org/docs/Web/API/ToggleEvent) */
interface ToggleEvent extends Event {
/** [MDN Reference](https://developer.mozilla.org/docs/Web/API/ToggleEvent/newState) */
readonly newState: string;
/** [MDN Reference](https://developer.mozilla.org/docs/Web/API/ToggleEvent/oldState) */
readonly oldState: string;
}

declare var ToggleEvent: {
prototype: ToggleEvent;
new(type: string, eventInitDict?: ToggleEventInit): ToggleEvent;
};

interface ToggleEventInit extends EventInit {
newState?: string;
oldState?: string;
}

// End TS >5.2
Copy link
Copy Markdown
Member Author

@rschristian rschristian Dec 19, 2024

Choose a reason for hiding this comment

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

We've never done this before (to my knowledge), but maybe it's a viable strategy? It's not too gnarly and hopefully with the blocks it can be removed in the future pretty easily.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

looks good to me. Admittedly I'm not even sure if we've settled on a particular TS version to support.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We haven't, though we do have split types for supporting >5.1 & <5.1 at the moment.

Issue is that every time we've bumped up the minimum people in other communities have had problems -- unfortunate side effect of being popular for widgets & component libs. IIUC, Angular still restricts TS versions per (Angular) major, for example.

@coveralls
Copy link
Copy Markdown

coveralls commented Dec 19, 2024

Coverage Status

coverage: 99.615%. remained the same
when pulling a463c9a on fix/on-toggle-types
into 3972db1 on main.

@rschristian rschristian marked this pull request as ready for review December 19, 2024 22:10
@rschristian rschristian merged commit 3618771 into main Dec 20, 2024
@rschristian rschristian deleted the fix/on-toggle-types branch December 20, 2024 19:18
@JoviDeCroock JoviDeCroock mentioned this pull request Dec 27, 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.

onToggle types do not use ToggleEvent

4 participants