maoberlehner / vue-lazy-hydration

Lazy Hydration of Server-Side Rendered Vue.js Components
MIT License
1.18k stars 52 forks source link

passive event listener to make lighthouse best practices happy #53

Closed danielkellyio closed 3 years ago

danielkellyio commented 4 years ago

Running google lighthouse audits I'm getting the following issue reported:

Screen Shot 2020-05-12 at 9 16 25 PM

From what I can tell from the learn more link events can be passive unless they have the possibility of preventing the event (which I don't see why lazy hydration would need to do)

Marking as passive is supposed to be better for performance, from what I understand.

Thanks!

spacedawwwg commented 4 years ago

+1 for this addition

maoberlehner commented 3 years ago

Thank you @danielkellyio and @Sshuichi for your contributions. What happens when passive is applied to an unsupported event?

asennoussi commented 3 years ago

Agreed, that's what I avoided in this PR https://github.com/maoberlehner/vue-lazy-hydration/pull/60 (Sorry I didn't know this PR existed before I created mine, this is why I added my review here. )

asennoussi commented 3 years ago

To answer your question @maoberlehner, if passive is applied to 'unsupported' event, it will never call The preventDefault() method which cancels the event if it is cancelable, meaning that the default action that belongs to the event will not occur.

maoberlehner commented 3 years ago

Closing this in favour of #60 Thanks @danielkellyio for getting the ball rolling.

maoberlehner commented 3 years ago

To answer your question @maoberlehner, if passive is applied to 'unsupported' event, it will never call The preventDefault() method which cancels the event if it is cancelable, meaning that the default action that belongs to the event will not occur.

@Sshuichi after thinking closer about it I'm still not sure what's the problem with using passive on all events?

asennoussi commented 3 years ago

It's hard to come up with an example but I'll try: Imagine you do your lazyhydration on a click event, if passive is there, you need to click twice, because the first event is canceled with passive.

I'm thinking of a link inside a component that is being lazyhydrated on click. If you click on that link, nothing will happen. the link will work on the second click though. 🤔

maoberlehner commented 3 years ago

That's a known issue that is not related to passive event listeners. See https://github.com/maoberlehner/vue-lazy-hydration/issues/40

asennoussi commented 3 years ago

Not really, I'm talking about an <a> link. That doesn't need JS to be hydrated, does it? Imagine you have an anchor link inside a component hydrated on click, the link should work even if the parent component is not hydrated or am I mistaken?

maoberlehner commented 3 years ago

Can you give a live example? Made a quick CodePen (with plain JavaScript) and the Link works as expected 🤔 https://codepen.io/maoberlehner/pen/ExPeRor

asennoussi commented 3 years ago

Yes, you are right. We need to test all events to make sure passive doesn't break any default behavior. But for precaution, let's do it only for scroll events since it's the one lighthouse is complaining about.

maoberlehner commented 3 years ago

I get your point but if there is no known disadvantage to it I prefer the simpler solution. Merging this and creating a new release later.

Thanks to all of you!

asennoussi commented 3 years ago

Thank you @maoberlehner for the fast review and interactivity and thanks to @danielkellyio for starting this! Any idea when this will be published on npm?

maoberlehner commented 3 years ago

Just published a new release!

asennoussi commented 3 years ago

I think we missed the fix for Lighthouse. There are two parts of eventListeners and we should add passive to interactions as well. Will create a PR for this.

asennoussi commented 3 years ago

Fixed here: https://github.com/maoberlehner/vue-lazy-hydration/pull/61