Skip to content

Svelte 5, an error should be thrown when event isn't defined #10393

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

Closed
rob-balfre opened this issue Feb 4, 2024 · 5 comments · Fixed by #10442
Closed

Svelte 5, an error should be thrown when event isn't defined #10393

rob-balfre opened this issue Feb 4, 2024 · 5 comments · Fixed by #10442

Comments

@rob-balfre
Copy link
Contributor

Describe the bug

When working with children snippets and event bubbling an error should be thrown if event isn't defined.

Since I was passing onfocus to the children snippet, the snippet needed to be defined explicitly so that onfocus is defined:

<TogglePopover>
+ {#snippet children(onfocus)}
    <input type="text" placeholder="Select Search" {onfocus} />
+ {/snippet}
</TogglePopover>

Reproduction

https://svelte-5-preview.vercel.app/#H4sIAAAAAAAAE5WRQWvDMAyF_4oQg6YQknubhu2426C7zTukjtIYHMs4SrcR8t9HmrRrNhjb0U9PT_rkHitjqcXNS4-uaAg3-OA9xigffny0J7JCGGPLXdCjkrU6GC-5ckpM4zkI7MmSlj0VQddQBW5glaS3YjLFrLbKZelXv8t8_ghvhRMQhkN3OFgCqQnYVay7FuhETsC45QRheObj0dITez5RyFI_pS1caY4xNlyaylCJGwkdDfEV8tb6T9rF7AlXYZIu5BlY4Q_ihe0cmxnnO4FxhZ1CoXdRCN4Wmmq2JYWdmreFaV2F0M8HGiDNxwHL0N-4F86_glsS6EHXxpaBHAywgzsf2LfRenupk6Vm1FsphCLXWXuuOYCqc1oMu8uvRrSGfmxTotm1bCmxfIxWBJs8h1UMNKUO307X3wdyJYXrItEcuB5-Er8On469s5naAgAA

Logs

No response

System Info

N/A

Severity

annoyance

@dummdidumm
Copy link
Member

Could you clarify what you expect to happen here? If I focus the input I already see an error in the console. Are you talking about wanting a red squiggly in your editor?

@dummdidumm dummdidumm added the awaiting submitter needs a reproduction, or clarification label Feb 5, 2024
@Prinzhorn
Copy link
Contributor

Prinzhorn commented Feb 5, 2024

Intuitively I'd expect an error that onfocus is not defined. But unfortunately it's a global and thus {onfocus} is essentially onfocus={window.onfocus}. Maybe all of them (event globals) should be excluded for Svelte? I don't see anyone actually using these in a meaningful way.

It becomes more clear if you do this:

<script>
	import TogglePopover from "./TogglePopover.svelte";

+	window.onfocus = console.log.bind(window, 'focus')
</script>

<TogglePopover>
	<input type="text" placeholder="Select Search" {onfocus} />
</TogglePopover>

@dummdidumm
Copy link
Member

Seems we're not the only ones with this problem, and there is no good solution in sight: microsoft/TypeScript#14306

@dummdidumm dummdidumm added types / typescript blocked by upstream and removed awaiting submitter needs a reproduction, or clarification labels Feb 6, 2024
@Prinzhorn
Copy link
Contributor

@dummdidumm how exactly is this related to TypeScript (serious question, the REPL doesn't use TypeScript)? In early Svelte versions you've kept a list of globals:

https://github.com/baseballyama/svelte/blob/dda3784cd9ca4e783d547ebeb3b7020996cdca09/src/compiler/utils/globals.ts#L708

if (globals.has(name) && node.type !== 'InlineComponent') return;

how does this work now? The globals.ts seems gone.

@dummdidumm
Copy link
Member

We removed the globals list because it resulted in too many false positives - in a world where everybody is using typed JavaScript it's better to defer to these tools instead.
That said, we didn't think of this case right here when we made the switch, and it may be worthwhile to introduce the inverse instead - a blocklist of globals on which we warn because the chance that you actually wanna use them like this is zero. That blocklist could contain all the on.. window events and we warn about it specifically.

Rich-Harris added a commit that referenced this issue Feb 17, 2024
* fix: warn against accidental global event referenced

closes #10393

* remove list

* remove else

* tweak

* update test

---------

Co-authored-by: Rich Harris <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants