feat: add no-add-event-listener rule
#1197
Conversation
|
Try the Instant Preview in Online PlaygroundInstall the Instant Preview to Your LocalPublished Instant Preview Packages:
|
| docs: { | ||
| description: 'Warns against the use of `addEventListener`', | ||
| category: 'Best Practices', | ||
| recommended: false, |
There was a problem hiding this comment.
Do you think this should be shipped as a recommended rule? (It might cause a lot of errors in existing code, though.)
cc: @ota-meshi
There was a problem hiding this comment.
ultimately i think it should be recommended, but im not sure how we can do that without introducing loads of errors like you say 🤔 it'd have to be in the next major i suppose
There was a problem hiding this comment.
Let’s make it recommended in the next major release.
| window.addEventListener ('message', handler); | ||
| // with a comment | ||
| window.addEventListener/* foo */('message', handler); |
There was a problem hiding this comment.
Should we also include the TypeScript case like this?
(window as any).addEventListener ('message', handler);
(window.addEventListener as any)('message', handler);There was a problem hiding this comment.
for this one, i added the test cases but this one isn't currently supported:
(window.addEventListener as any)('message', handler);we would need logic to handle call expressions containing casts, which feels weird. maybe we should just ignore this case and not try handle it?
There was a problem hiding this comment.
I’m not sure how complicated this is, so I’ll try implementing it myself later.
.../eslint-plugin-svelte/tests/fixtures/rules/no-add-event-listener/invalid/test01-input.svelte
Show resolved
Hide resolved
43081j
force-pushed
the
listeners-and-such
branch
from
a11babd to
e13635b
Compare
| window.addEventListener('resize', handler); | ||
| </script> | ||
| ``` | ||
|
|
There was a problem hiding this comment.
Can you add these 2 links in Further reading section? (IIRC, the original template has "Further reading" section.)
https://svelte.dev/docs/svelte/basic-markup#Events-Event-delegation
https://svelte.dev/docs/svelte/svelte-events#on
|
|
||
| ## :book: Rule Details | ||
|
|
||
| This rule reports usages of `addEventListener`: |
There was a problem hiding this comment.
Is it better?
| This rule reports usages of `addEventListener`: | |
| Svelte relies on event delegation for performance and predictable handler order. Calling `addEventListener` inside a component skips this mechanism. This rule reports any call to `addEventListener` suggests converting to the `on()` helper from `svelte/events`. |
| description: 'Warns against the use of `addEventListener`', | ||
| category: 'Best Practices', | ||
| recommended: false, | ||
| default: 'warn' |
There was a problem hiding this comment.
Why was the default set to warn?
| window.addEventListener ('message', handler); | ||
| // with a comment | ||
| window.addEventListener/* foo */('message', handler); |
There was a problem hiding this comment.
I’m not sure how complicated this is, so I’ll try implementing it myself later.
| const { callee, arguments: args } = node; | ||
| let target: string | null = null; | ||
|
|
||
| if (args.length < 2 || args.length > 3) { |
There was a problem hiding this comment.
Can we check such weird pattern? 😅
<script>
const foo = ['resize', () => { console.log('resized!') }]
window.addEventListner(...foo);
</script>There was a problem hiding this comment.
Even if suggestions aren’t possible, maybe it’s fine to just report any use of addEventListener.
this helped me learn about the events helpers, so figured i may as well throw it together.
let me know if there's anything i missed, as i don't think i've added a new rule in this repo before.
one thing to note is that it has a suggestion, but that relies on the user later making sure
onhas been imported correctly. im not sure we can do that automatically since each suggestion runs individually and should probably only affect the subset of code you're reportingclose: #1194