stencil-community / stencil-eslint

ESLint rules specific to Stencil JS projects
MIT License
57 stars 31 forks source link

How to resolve "Use vDOM listener instead" #10

Open chris-dura opened 4 years ago

chris-dura commented 4 years ago

Noobie question — I’m using an example straight from the Stencil docs…

@Listen('keydown')
handleKeyDown(ev: KeyboardEvent){
  if (ev.key === 'ArrowDown'){
    console.log('down arrow pressed')
  }
}

I get an error Use vDOM listener instead, and I don’t exactly know how to resolve that… 😬 In other words, I don't exactly know how to "use a vDOM listener instead".

Also, according to the rule docs, this rule is meant to "Ensures that no vdom events are used in @Listen decorator." Why is it bad to listen to vDOM events using the @Listen decorator?

VincentN commented 4 years ago

Running into this as well... Can someone point us in the right direction on how to fix that problem/example? The docs use this example too, so something might need fixing there as well? I'm willing to open a PR there if needed. Pinging @adamdbradley or @manucorporat

VincentN commented 4 years ago

Ok, according to the StencilJS docs example, using @Listen('keydown') should work fine but this approach resolves the error:

    @Element() el: HTMLElement;

    /**
     * Handle the key press
     */
    handleKeydown(event: KeyboardEvent){
        // do stuff here
    }

    connectedCallback(){
        this.el.addEventListener('keydown', this.handleKeydown);
    }

    disconnectedCallback(){
        this.el.removeEventListener('keydown', this.handleKeydown);
    }

Should the Stencil docs be updated to reflect this updated guideline?

chris-dura commented 4 years ago

@VincentN -- So, couple questions...

  1. A big benefit of using StencilJS is the convenience of all those decorators. Do you have any idea why using the syntactic sugar of @Listen() would be considered bad practice? Obviously, it's not a lot of extra boilerplate... but, if it "should work fine" (and I don't understand why it should be avoided with listeners), I'm strongly considering just disabling the rule to keep the code a little cleaner 😬

  2. In your example, I assume a @Element() el: HTMLElement; is needed as well, correct?

Should the Stencil docs be updated to reflect this updated guideline?

I think adding event listeners is so ubiquitous, the docs should definitely have an example of the preferred method... and if that's not using the @Listen decorator, then there should definitely be a valid @Listen example replacing the current one.

VincentN commented 4 years ago

Hi @chris-dura, legitimate questions.

  1. I have don't really have an idea why stencil-eslint would prevent the use of the @Listen decorator when handling keyboard events... It sure misses the convenience that Stencil offers.
  2. That's correct. I forgot to include that bit.

I'm hoping someone with more intimate knowledge of this linting rule can clear up this confusion...

TRMW commented 3 years ago

I just ran into this as well, and am wondering if this rule is actually correct. Looks like it was added in #2 by @d0whc3r and merged by @manucorporat. There is no discussion on the PR. Could one of you chime in on the thinking behind this rule?

d0whc3r commented 3 years ago

I just ran into this as well, and am wondering if this rule is actually correct. Looks like it was added in #2 by @d0whc3r and merged by @manucorporat. There is no discussion on the PR. Could one of you chime in on the thinking behind this rule?

Well, this rule is originally from the stencil-eslint package my pr only updates code style, and there is a documentation for this rule, https://github.com/ionic-team/stencil-eslint/blob/master/docs/prefer-vdom-listener.md this rule refers that you better define "vdom-events" in tsx code instead of using @Listen.

You always can disable the rule in you eslint config

chris-dura commented 3 years ago

You always can disable the rule in you eslint config

@d0whc3r -- Yes, of course. But, I think there's still a couple points that haven't really been addressed by someone "in-the-know"...

In particular, the reasoning for the rule (ie, why is using @Listen for "vDOM events" not recommended) isn't documented anywhere I've been able to find. The rule presents like it is something more than a "code style preference"... the messaging makes it seem like it could cause negative impact on code performance or actual bugs... unlike rules that just deal with things like whitespace?

(Maybe that's where I'm mistaken -- perhaps it is just a code style preference, but that seems like a weird default since @Listen makes things much easier to read 🤷)

But, also, imo, having examples in the docs that break the rules potentially can (and in my case has 😅) caused some confusion for new users. Especially when there's no direction on the recommended way to listen to "vDOM events", if we're not supposed to use @Listen 🤷

d0whc3r commented 3 years ago

Documentation was written before stencil-eslint and maybe it needs to be reviewed otherwise when you use events in "html" code it will be checked by typescript and maybe in some components you will see that event is wrong this could help to catch some issues

elwynelwyn commented 3 years ago

Have hit the lint error and this issue is the top google result, and I am still unsure why this rule exists and what sorts of issues it is intended to catch.

I would love some extra docs on this as well if anyone knowledgeable can contribute.

adrianschmidt commented 3 years ago

This is just a guess, but have @Listen always had the option to register passive events? If not, that could be the reason the rule was once created. Precisely because @Listen is so much easier than manually registering event listeners, people might have been falling for the attraction of using "active" ("non-passive"?) listeners with @Listen instead of using passive listeners because registering them was more complicated…

I just ran into this as well, and am wondering if this rule is actually correct. Looks like it was added in #2 by @d0whc3r and merged by @manucorporat. There is no discussion on the PR. Could one of you chime in on the thinking behind this rule?

Well, this rule is originally from the stencil-eslint package my pr only updates code style, and there is a documentation for this rule, https://github.com/ionic-team/stencil-eslint/blob/master/docs/prefer-vdom-listener.md this rule refers that you better define "vdom-events" in tsx code instead of using @Listen.

You always can disable the rule in you eslint config

As far as I can find, it was originally added here: https://github.com/ionic-team/stencil-eslint/commit/f72d4728157cc68928e4afe8caf9b97c6d143438#diff-1c48f86a4f47de44e085c3d33a0106e365eae17ee5e81b2b648a463551d0dfab

The description at that time was "This rule catches Stencil Props marked as private or protected.", but that seems to just be a copy-paste mistake from this file: https://github.com/ionic-team/stencil-eslint/blob/f72d4728157cc68928e4afe8caf9b97c6d143438/src/rules/methods-must-be-public.ts

borisdiakur commented 3 years ago

This is just a guess, but have @Listen always had the option to register passive events? If not, that could be the reason the rule was once created. Precisely because @Listen is so much easier than manually registering event listeners, people might have been falling for the attraction of using "active" ("non-passive"?) listeners with @Listen instead of using passive listeners because registering them was more complicated…

This guess makes totally sense to me. I'm now "silencing" the error message by providing the passive argument to the decorator:

@Listen('keydown', {
  passive: true,
})
handleKeyDown(ev: KeyboardEvent) {
  // ...
}

Note that you can also pass { passive: false } or even {} to silence the warning as the related rule condition evaluates to false if any additional argument is provided (smells like a bug): https://github.com/ionic-team/stencil-eslint/commit/f72d4728157cc68928e4afe8caf9b97c6d143438#diff-1c48f86a4f47de44e085c3d33a0106e365eae17ee5e81b2b648a463551d0dfabR21

I also learnt that I need to specify a target in the options in order for the event to be triggered outside my component: https://stenciljs.com/docs/events#target And here again, just adding this option also resolves the error message.

I would sleep better though if there was someone who could clarify the error message intent.

ionitron-bot[bot] commented 2 years ago

Thanks for the issue! This issue is being closed due to inactivity. If this is still an issue with the latest version of Stencil, please create a new issue and ensure the template is fully filled out.

Thank you for using Stencil!

rwaskiewicz commented 2 years ago

Ah, some of these issues are so old that they're getting cleared automatically. Sorry about that!