Skip to content

Conversation

@deokjinkim
Copy link
Contributor

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

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
ContributorAuthor

@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

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

@deokjinkim
Copy link
ContributorAuthor

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

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 :)

@deokjinkimdeokjinkim reopened this Jan 7, 2023
@deokjinkimdeokjinkim changed the title events: add initEvent to Event[draft]events: add initEvent to EventJan 7, 2023
Copy link
Member

@benjamingrbenjamingr 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

@deokjinkimdeokjinkim changed the title [draft]events: add initEvent to Eventevents: add initEvent to EventJan 9, 2023
@deokjinkim
Copy link
ContributorAuthor

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

Copy link
Member

@anonriganonrig 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

@mcollinamcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@ronagronag 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-actionsgithub-actionsbot 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
ContributorAuthor

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

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

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.

Co-authored-by: Antoine du Hamel <[email protected]>
@aduh95aduh95 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-actionsgithub-actionsbot 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

@lpincalpinca added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 19, 2023
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 19, 2023
@nodejs-github-botnodejs-github-bot merged commit 2ff8c50 into nodejs:mainJan 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 <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Zeyu "Alex" Yang <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
@RafaelGSSRafaelGSS mentioned this pull request Jan 20, 2023
@juanarbol
Copy link
Member

This is not landing cleanly in v18.x

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.commit-queue-squashAdd 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.

13 participants

@deokjinkim@anonrig@benjamingr@annevk@nodejs-github-bot@aduh95@juanarbol@mcollina@jasnell@ronag@himself65@RafaelGSS@lpinca