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.

Sign up for GitHub

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

fix: Incorrect onToggle event type #4615

Merged
merged 2 commits into from
Dec 20, 2024
Merged

fix: Incorrect onToggle event type #4615

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
98308a1
fix: Incorrect onToggle event type
rschristian Dec 19, 2024
a463c9a
refactor: Backport ToggleEvent
rschristian Dec 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 30 additions & 1 deletion src/jsx.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,28 @@ type Defaultize<Props, Defaults> =

type Booleanish = boolean | 'true' | 'false';

// 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
Comment on lines +23 to +43
Copy link
Member Author

@rschristian rschristian Dec 19, 2024 •
edited
Loading

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
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
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.


export namespace JSXInternal {
export type LibraryManagedAttributes<Component, Props> = Component extends {
defaultProps: infer Defaults;
Expand Down Expand Up @@ -508,6 +530,10 @@ export namespace JSXInternal {
Target,
TouchEvent
>;
export type TargetedToggleEvent<Target extends EventTarget> = TargetedEvent<
Target,
ToggleEvent
>;
export type TargetedTransitionEvent<Target extends EventTarget> =
TargetedEvent<Target, TransitionEvent>;
export type TargetedUIEvent<Target extends EventTarget> = TargetedEvent<
Expand Down Expand Up @@ -536,6 +562,9 @@ export namespace JSXInternal {
export type DragEventHandler<Target extends EventTarget> = EventHandler<
TargetedDragEvent<Target>
>;
export type ToggleEventHandler<Target extends EventTarget> = EventHandler<
TargetedToggleEvent<Target>
>;
export type FocusEventHandler<Target extends EventTarget> = EventHandler<
TargetedFocusEvent<Target>
>;
Expand Down Expand Up @@ -597,7 +626,7 @@ export namespace JSXInternal {
onCompositionUpdateCapture?: CompositionEventHandler<Target> | undefined;

// Details Events
onToggle?: GenericEventHandler<Target> | undefined;
onToggle?: ToggleEventHandler<Target> | undefined;

// Dialog Events
onClose?: GenericEventHandler<Target> | undefined;
Expand Down
12 changes: 10 additions & 2 deletions test/ts/preact.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -351,6 +351,10 @@ const acceptsStringAsLength = <div style={{ marginTop: '20px' }} />;

const ReturnNull: FunctionalComponent = () => null;

// Should accept arbitrary properties outside of JSX.HTMLAttributes
h('option', { x: 'foo' });
createElement('option', { value: 'foo' });

// Refs should work on elements
const ref = createRef<HTMLDivElement>();
createElement('div', { ref: ref }, 'hi');
Expand All @@ -375,13 +379,17 @@ const onBeforeInput = (e: h.JSX.TargetedInputEvent<HTMLInputElement>) => {};
createElement('input', { onBeforeInput: onBeforeInput });
h('input', { onBeforeInput: onBeforeInput });

// Should accept onSubmit
const onSubmit = (e: h.JSX.TargetedSubmitEvent<HTMLFormElement>) => {};
<form onSubmit={e => e.currentTarget.elements} />;
createElement('form', { onSubmit: onSubmit });
h('form', { onSubmit: onSubmit });

h('option', { value: 'foo' });
createElement('option', { value: 'foo' });
Comment on lines -383 to -384
Copy link
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.

// Should accept onToggle
const onToggle = (e: h.JSX.TargetedToggleEvent<HTMLDetailsElement>) => {};
<dialog onToggle={(e) => ({ newState: e.newState, oldState: e.oldState }) } />;
createElement('dialog', { onToggle: onToggle });
h('dialog', { onToggle: onToggle });

// Should default to correct event target element for the attribute interface
h<JSX.InputHTMLAttributes>('input', { onClick: e => e.currentTarget.capture });
Expand Down
Loading