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: expose Event statics #33649

Closed
wants to merge 3 commits into from
Closed

Conversation

benjamingr
Copy link
Member

@benjamingr benjamingr commented May 30, 2020

Expose Event statics (Event.NONE etc). See Event phase constants.

  • Chrome, Firefox, Edge, and Safari implement these.
  • IE11 implements all of these except NONE (for some reason).

I think these are required on the event interface (as constants).

They are as-enumerable as they are in browsers though I did not add a specific test for enumerability (I can if people feel strongly about it)

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 added the events Issues and PRs related to the events subsystem / EventEmitter. label May 30, 2020
@benjamingr
Copy link
Member Author

Also I did not fix the tests at the top that check eventPhase with the numeric values (rather than constants) to not create coupling, but I can happily change that.

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 30, 2020
@nodejs-github-bot
Copy link
Collaborator

static NONE = 0;
static CAPTURING_PHASE = 1;
static AT_TARGET = 2;
static BUBBLING_PHASE = 3;
Copy link
Member

Choose a reason for hiding this comment

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

Adding a comment in here that we currently do not use the CAPTURING_PHASE and BUBBLING_PHASE but that they need to be preserved anyway because of the spec would be good.

@BridgeAR BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:18
@nodejs-github-bot
Copy link
Collaborator

@jasnell jasnell added eventtarget Issues and PRs related to the EventTarget implementation. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Jun 19, 2020
@nodejs-github-bot
Copy link
Collaborator

@jasnell
Copy link
Member

jasnell commented Jun 21, 2020

Adding a fixup commit resolving the rebase conflict.

@jasnell
Copy link
Member

jasnell commented Jun 21, 2020

@benjamingr ... can I ask to you squash the commits here?

@benjamingr
Copy link
Member Author

Sure though I have a computer without built node so I am doing everything "blind" until early next week.

@benjamingr
Copy link
Member Author

@jasnell I see there is a merge commit present - I assume that's not intentional? Can I pluck it out and squash the other two?

@jasnell
Copy link
Member

jasnell commented Jun 21, 2020

Actually, don't worry about squashing or fixing up. I'm combining all these PRs into a single PR to make landing them easier

@benjamingr
Copy link
Member Author

@jasnell to be explicit, you have my explicit consent (and blessing) to do as you wish with the code here and in the other PRs (and in follow up event-target PRs when I get back to my workstation next week).

@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

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