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

How to resolve "Use vDOM listener instead" #10

Open
chris-dura opened this issue Apr 6, 2020 · 13 comments
Open

How to resolve "Use vDOM listener instead" #10

chris-dura opened this issue Apr 6, 2020 · 13 comments
Labels

Comments

@chris-dura
Copy link

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
Copy link

VincentN commented Jul 6, 2020

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
Copy link

VincentN commented Jul 6, 2020

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
Copy link
Author

chris-dura commented Jul 8, 2020

@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
Copy link

VincentN commented Jul 9, 2020

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
Copy link

TRMW commented Aug 13, 2020

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
Copy link
Contributor

d0whc3r commented Aug 13, 2020

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
Copy link
Author

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
Copy link
Contributor

d0whc3r commented Aug 15, 2020

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
Copy link

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
Copy link

adrianschmidt commented Dec 9, 2020

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: f72d472#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
Copy link

borisdiakur commented Apr 10, 2021

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): f72d472#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
Copy link

ionitron-bot bot commented Oct 12, 2021

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!

@ionitron-bot ionitron-bot bot closed this as completed Oct 12, 2021
@ionitron-bot ionitron-bot bot locked and limited conversation to collaborators Oct 12, 2021
@rwaskiewicz
Copy link
Contributor

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

8 participants