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 EventListener validation #43491

Merged
merged 1 commit into from
Jun 29, 2022

Conversation

daeyeon
Copy link
Member

@daeyeon daeyeon commented Jun 19, 2022

This fixes validating EventListener given to addEventListener to improve the compatibility.

According to the WPT test result failed with the current validation, addEventListener
should not require handleEvent to be defined on an EventListener. IIUC, the same can
be applied to removeEventListener also.

Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com

This fixes validating `EventListener` given to `add/removeEventListener`
to improve the Web API compatibility.

According to the WPT test failure with the current validation,
`addEventListener` should not require `handleEvent` to be defined on
an `EventListener`".  IIUC, the same can be applied to
`removeEventListener` also.

Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com
@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Jun 19, 2022
Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

You learn something new every day, had no idea this works.

Thanks for the fix.

@benjamingr benjamingr added request-ci Add this label to start a Jenkins CI on a PR. confirmed-bug Issues with confirmed bugs. labels Jun 20, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 20, 2022
@nodejs-github-bot
Copy link
Collaborator

@daeyeon
Copy link
Member Author

daeyeon commented Jun 20, 2022

@benjamingr Thanks for your kind feedback. :-)

@nodejs-github-bot
Copy link
Collaborator

@Ayase-252 Ayase-252 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 28, 2022
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@benjamingr benjamingr added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 29, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 29, 2022
@nodejs-github-bot nodejs-github-bot merged commit ed1e9ae into nodejs:main Jun 29, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in ed1e9ae

@benjamingr
Copy link
Member

Congrats 🎉

@daeyeon
Copy link
Member Author

daeyeon commented Jun 29, 2022

Thank you @benjamingr and thank you all!

@daeyeon daeyeon deleted the main.events-220619.Sun.5c32 branch June 29, 2022 12:05
mabaasit pushed a commit to mabaasit/node that referenced this pull request Jul 6, 2022
This fixes validating `EventListener` given to `add/removeEventListener`
to improve the Web API compatibility.

According to the WPT test failure with the current validation,
`addEventListener` should not require `handleEvent` to be defined on
an `EventListener`".  IIUC, the same can be applied to
`removeEventListener` also.

Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com

PR-URL: nodejs#43491
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Jul 12, 2022
This fixes validating `EventListener` given to `add/removeEventListener`
to improve the Web API compatibility.

According to the WPT test failure with the current validation,
`addEventListener` should not require `handleEvent` to be defined on
an `EventListener`".  IIUC, the same can be applied to
`removeEventListener` also.

Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com

PR-URL: #43491
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Jul 20, 2022
This fixes validating `EventListener` given to `add/removeEventListener`
to improve the Web API compatibility.

According to the WPT test failure with the current validation,
`addEventListener` should not require `handleEvent` to be defined on
an `EventListener`".  IIUC, the same can be applied to
`removeEventListener` also.

Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com

PR-URL: #43491
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Jul 31, 2022
This fixes validating `EventListener` given to `add/removeEventListener`
to improve the Web API compatibility.

According to the WPT test failure with the current validation,
`addEventListener` should not require `handleEvent` to be defined on
an `EventListener`".  IIUC, the same can be applied to
`removeEventListener` also.

Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com

PR-URL: #43491
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
This fixes validating `EventListener` given to `add/removeEventListener`
to improve the Web API compatibility.

According to the WPT test failure with the current validation,
`addEventListener` should not require `handleEvent` to be defined on
an `EventListener`".  IIUC, the same can be applied to
`removeEventListener` also.

Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com

PR-URL: nodejs/node#43491
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. confirmed-bug Issues with confirmed bugs. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants