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

(fix) keep colon while transforming events #1549

Merged
merged 2 commits into from Aug 26, 2022

Conversation

dummdidumm
Copy link
Member

on:click -> "on:click" instead of on:click -> "onclick"

Related to sveltejs/svelte#7649 - @jasonlyu123 this is what you had in mind, right?

on:click -> "on:click" instead of on:click -> "onclick"
@jasonlyu123
Copy link
Member

Should we keep it extend the svelte.JSX.DOMAttributes for a little while? Because this would probably break the custom DOM typedef in the new transformation. Or we can create some migration guide somewhere and do it now?

@dummdidumm
Copy link
Member Author

Good point. I think we need some migration guide either way because if we go with on:click etc people defining custom events need to add the colon to their type definitions even if we extend from the previous definitions. Damn this situation sucks.. I don't see a way how we can magically find out whether or not to add the colon for a unknown event.

@dummdidumm
Copy link
Member Author

dummdidumm commented Jul 6, 2022

Ok I found a way to make this work by using a new TS transformation:

type EventsWithColon<T> = {[Property in keyof T as Property extends `on${infer Key}` ? `on:${Key}` : never]?: T[Property] }

This transforms all onX properties to on:X. Since this requires a relatively new TS version I suggest to keep this PR open until we do the switch from the old to the new transformation, which is a breaking change anyway, and we bump the minimum required TS version along with it.

Edit: Not sure anymore if we should do this, see roadmap.

Edit 2: I just realized that we have access to the namespace name. So we could detect if it's svelteNative or svelteNodeGUI and not add the colon in this case. This gridlocks them to always use a different event naming scheme though.

Edit 3: If libraries do "this component extends the typings of svelte2tsx.HTMLAttributes" (some prominent ones like Carbon Svelte do this) their usage may be broken with the new transformation since these have the onX typings, not the on:X typings. I'm not 100% if it breaks though since if they use SvelteComponentTyped they need to have and extra field for events - we may be lucky here after all and this isn't a problem.

Edit 4: I think we should go with this after all, see sveltejs/svelte#7649 (comment) for reasoning.

@dummdidumm dummdidumm mentioned this pull request Jul 6, 2022
9 tasks
@dummdidumm dummdidumm marked this pull request as draft July 6, 2022 10:37
@dummdidumm dummdidumm marked this pull request as ready for review August 26, 2022 14:43
@dummdidumm dummdidumm merged commit cdd046a into sveltejs:master Aug 26, 2022
@dummdidumm dummdidumm deleted the events-transformation branch August 26, 2022 14:43
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

2 participants