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

events: improve argument handling, start passive #33637

Closed

Conversation

benjamingr
Copy link
Member

Hey, this PR fixes argument handling for cases like addEventListener('foo', undefined) which is allowed as well as add a whatwg WPT test that tests this.

Note the actual passive tests don't pass (because we don't deal with passive).

IIUC passive means "if event.preventDefault is called from within a passive listener - don't preventDefault". I will make a PR fixing this after #33613 lands.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@benjamingr benjamingr requested a review from jasnell May 29, 2020 11:45
@benjamingr benjamingr added the events Issues and PRs related to the events subsystem / EventEmitter. label May 29, 2020
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this might be spec compliant it's not ideal to allow this. It'll hide potential bugs.

@benjamingr
Copy link
Member Author

benjamingr commented May 29, 2020

@BridgeAR I see your point and thank your for championing developer experience and (what I consider) good default behavior for passing invalid arguments.

I don't think it's our place to make this call if we ship EventTarget, we should strive to make our EventTarget as spec compliant as possible (like our URL) within reasonable limitations.

This PR is a prerequisite for running (and passing) the WPTs for events. If you feel strongly about diverging from the spec here - I am fine with making NodeEventTarget (our extensions) behave differently from the spec although I'd argue that's more confusing.

I feel people generally would expect an EventTarget to behave in Node the same way it does in browsers . The spec allows passing null (and undefined by idl conversions IIUC).

I think we are probably going to find more fundamental issues with EventTarget (like the error handling behavior, where I would particularly value your opinion and feedback) - and we should probably pick our battles here if/when we want to diverge from the spec or ask for it to be amended.

@benjamingr
Copy link
Member Author

@BridgeAR I checked with Anne on IRC, it sounds like emitting a warning is allowed from a spec point of view. Would that address your concern?

(for reference here is my conversation with Anne)

<benjamingr>Just checking, how bad would it be for Node to behave differently from `EventTarget` regarding `addEventListener('eventName', null)`? https://github.com/nodejs/node/pull/33637#pullrequestreview-420889677
3:27 PM <annevk> benjamingr: do you have a developer console equivalent?
3:28 PM <benjamingr> Benjamin Gruenbaum annevk: We have `stdout` (and stderr for warnings) :] 
3:28 PM <annevk> I'd use that; I think as Domenic mentioned elsewhere, if you're not going for full compatibility, it'll just be confusing
3:29 PM <benjamingr> Use that to do what? Emit a warning for example? 
3:29 PM <annevk> benjamingr__: yes
3:30 PM <benjamingr> I  generally agree with Domenic (and that PoV). Would Node.js writing a warning to `stderr` for passing `undefined` or `null` as a second argument be allowed from a spec point of view?
3:31 PM <annevk> benjamingr__: yeah, console is UI territory
3:31 PM <benjamingr__> Ok great, thanks again. I'll go ahead and suggest that in the issue and see if it helps.

@benjamingr
Copy link
Member Author

@BridgeAR I've updated the code to emit a warning on incorrect usage, PTAL.

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like it for the same reasons as @BridgeAR but... Ok

@BridgeAR
Copy link
Member

@benjamingr thanks for adding the warning. I agree that there might be more issues that we find and we should probably open a tracking issue for things where we believe the user experience is not ideal. That way it would be easier to determine what we could/should bring up to discuss to improve in the spec itself.

@benjamingr benjamingr force-pushed the event-passive-listener-argumetns branch from 706cd34 to 73058fb Compare May 30, 2020 11:36
@BridgeAR BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:18
@benjamingr benjamingr force-pushed the event-passive-listener-argumetns branch from 73058fb to e985ec7 Compare June 6, 2020 09:02
@benjamingr
Copy link
Member Author

@jasnell this was out of sync, rebased this and the others

@nodejs-github-bot
Copy link
Collaborator

@ronag
Copy link
Member

ronag commented Jun 17, 2020

Any chance we could use a different submodule prefix for EventTarget? I always get confused whether these commits are regarding event emitter or event target... especially since the description often can at least partly apply to either.

@jasnell
Copy link
Member

jasnell commented Jun 22, 2020

Closing in favor of #34015 (which combines this and other eventtarget PRs into a single to make landing easier

@jasnell jasnell closed this Jun 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
events Issues and PRs related to the events subsystem / EventEmitter. eventtarget Issues and PRs related to the EventTarget implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants