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: add initEvent to Event #46069

Merged
merged 5 commits into from
Jan 19, 2023

Conversation

deokjinkim
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Jan 3, 2023
@anonrig
Copy link
Member

anonrig commented Jan 3, 2023

Mdn states that;

Deprecated: This feature is no longer recommended. Though some browsers might still support it, it may have already been removed from the relevant web standards, may be in the process of being dropped, or may only be kept for compatibility purposes. Avoid using it, and update existing code if possible; see the compatibility table at the bottom of this page to guide your decision. Be aware that this feature may cease to work at any time.

@deokjinkim
Copy link
Contributor Author

@anonrig Thank you for checking. BTW, I have one question. mdn states that cancelBubble, returnValue, and srcElement are also deprecated, but as I know Event class has these properties for now. So do we need to remove these properties?

Refs: https://developer.mozilla.org/en-US/docs/Web/API/Event/cancelBubble
Refs: https://developer.mozilla.org/en-US/docs/Web/API/Event/returnValue
Refs: https://developer.mozilla.org/en-US/docs/Web/API/Event/srcElement

@anonrig
Copy link
Member

anonrig commented Jan 4, 2023

I believe it will be a breaking change. We should deprecate them, if it’s not, through documentation.

@deokjinkim
Copy link
Contributor Author

Thank you for feedback. I'll close this PR soon.

@benjamingr
Copy link
Member

I actually disagree with @anonrig here. If the spec defines initEvent on EventTarget we have to implement it in order to be spec compliant.

And indeed we fail the relevant WPT tests.

@benjamingr
Copy link
Member

Let's ask some WHATWG folk - @domenic @annevk should we implement initEvent (and other stuff tagged legacy) or is it fine not to?

@annevk
Copy link

annevk commented Jan 6, 2023

Browsers definitely have to keep supporting it and I think what MDN states is stretching reality. It's probably easier for web developers if Node.js supports it as well so they don't have to adapt code, but I could see an argument for not implementing it.

@benjamingr
Copy link
Member

@deokjinkim feel free to reopen then :)

@deokjinkim deokjinkim reopened this Jan 7, 2023
@deokjinkim deokjinkim changed the title events: add initEvent to Event [draft]events: add initEvent to Event Jan 7, 2023
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.

Would be good to add more tests, thanks

@deokjinkim deokjinkim changed the title [draft]events: add initEvent to Event events: add initEvent to Event Jan 9, 2023
@deokjinkim
Copy link
Contributor Author

@benjamingr Thank you for review. Added description of event.initEvent to doc. fa88cdb

Copy link
Member

@anonrig anonrig left a comment

Choose a reason for hiding this comment

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

I'm still not in favor of adding a deprecated API to Node.js without any request from a user, just for the goal of spec compliance.

I couldn't find a past example of adding a deprecated API to Node.js. It's not a blocker, but I want to ask @nodejs/tsc opinion about this before merging.

@benjamingr
Copy link
Member

I'm still not in favor of adding a deprecated API to Node.js without any request from a user, just for the goal of spec compliance.

With web standards I feel that you either implement the standard or you don't. This isn't an API like EventEmitter where we get to do as we please - with EventTarget I feel strongly that changes to the API need to be reflected by changes to the spec.

I can definitely see an argument for amending the WHATWG DOM spec (which project members have done in the past) to make initEvent optional (by adding it to an annex clients may implement or making it an extension e.g.).

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@ronag ronag added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed needs-ci PRs that need a full CI run. labels Jan 10, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 10, 2023
@nodejs-github-bot
Copy link
Collaborator

@deokjinkim
Copy link
Contributor Author

Rebased this PR to fix conflict and squashed to 1 commit.

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 11, 2023
@aduh95
Copy link
Contributor

aduh95 commented Jan 13, 2023

I couldn't find a past example of adding a deprecated API to Node.js.

We've added atob and btoa, marked as Legacy since their initial implementation in Node.js.

doc/api/events.md Outdated Show resolved Hide resolved
doc/api/events.md Outdated Show resolved Hide resolved
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
doc/api/events.md Outdated Show resolved Hide resolved
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
@aduh95 aduh95 added request-ci Add this label to start a Jenkins CI on a PR. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Jan 16, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 16, 2023
@nodejs-github-bot
Copy link
Collaborator

@lpinca lpinca added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 19, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 19, 2023
@nodejs-github-bot nodejs-github-bot merged commit 2ff8c50 into nodejs:main Jan 19, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 2ff8c50

RafaelGSS pushed a commit that referenced this pull request Jan 20, 2023
Refs: https://dom.spec.whatwg.org/#dom-event-initevent
PR-URL: #46069
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Jan 20, 2023
@juanarbol juanarbol added the backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. label Jan 26, 2023
@juanarbol
Copy link
Member

This is not landing cleanly in v18.x

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. backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet