GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — events: add `initEvent` to Event by deokjinkim · Pull Request #46069 · nodejs/node

Conversation

@deokjinkim

@deokjinkim deokjinkim commented Jan 3, 2023

Copy link
Copy Markdown
Contributor

Refs: https://dom.spec.whatwg.org/#dom-event-initevent

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

anonrig commented Jan 3, 2023

Copy link
Copy Markdown
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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@deokjinkim

deokjinkim commented Jan 4, 2023

Copy link
Copy Markdown
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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@anonrig

anonrig commented Jan 4, 2023

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@deokjinkim

deokjinkim commented Jan 4, 2023

Copy link
Copy Markdown
Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@deokjinkim deokjinkim closed this Jan 4, 2023
@deokjinkim deokjinkim mentioned this pull request Jan 4, 2023
events: change status of event.srcElement to legacy #46085
Merged
@benjamingr

benjamingr commented Jan 6, 2023

Copy link
Copy Markdown
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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@benjamingr

benjamingr commented Jan 6, 2023

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@annevk

annevk commented Jan 6, 2023

Copy link
Copy Markdown

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 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@benjamingr

benjamingr commented Jan 6, 2023

Copy link
Copy Markdown
Member

@deokjinkim feel free to reopen then :)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@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
@deokjinkim
deokjinkim force-pushed the 230103_add_initevent branch from 95c95d5 to f5974f6 Compare January 7, 2023 01:57
benjamingr
benjamingr approved these changes Jan 7, 2023
View reviewed changes

@benjamingr benjamingr left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@deokjinkim deokjinkim changed the title [draft]events: add initEvent to Event events: add initEvent to Event Jan 9, 2023
@deokjinkim

deokjinkim commented Jan 9, 2023

Copy link
Copy Markdown
Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

anonrig
anonrig reviewed Jan 9, 2023
View reviewed changes

@anonrig anonrig left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
ronag
ronag approved these changes Jan 9, 2023
View reviewed changes
@benjamingr

benjamingr commented Jan 9, 2023

Copy link
Copy Markdown
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.).

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

mcollina
mcollina approved these changes Jan 10, 2023
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

lgtm

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@ronag ronag added request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. author ready PRs with CI started, the required approvals, and no outstanding review comments. 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. Only starts once the PR has an approving review. label Jan 10, 2023
@nodejs-github-bot

nodejs-github-bot commented Jan 10, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/48926/

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@deokjinkim
events: add initEvent to Event
7fdc27d
Refs: https://dom.spec.whatwg.org/#dom-event-initevent
@deokjinkim
deokjinkim force-pushed the 230103_add_initevent branch from fa88cdb to 7fdc27d Compare January 11, 2023 01:20
@deokjinkim

deokjinkim commented Jan 11, 2023

Copy link
Copy Markdown
Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jan 11, 2023
himself65
himself65 approved these changes Jan 13, 2023
View reviewed changes
@aduh95

aduh95 commented Jan 13, 2023

Copy link
Copy Markdown
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.

anonrig and deokjinkim reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

anonrig
anonrig approved these changes Jan 13, 2023
View reviewed changes
RafaelGSS
RafaelGSS approved these changes Jan 13, 2023
View reviewed changes
jasnell
jasnell reviewed Jan 13, 2023
View reviewed changes
Comment thread doc/api/events.md Resolved

Uh oh!

There was an error while loading. Please reload this page.

This was referenced Jan 14, 2023
CI Reliability 2023-01-14 nodejs/reliability#483
Open
CI Reliability 2023-01-15 nodejs/reliability#484
Open
CI Reliability 2023-01-16 nodejs/reliability#485
Open
@deokjinkim
update description to clarify status of the WHATWG spec
c0df63b
aduh95
aduh95 reviewed Jan 16, 2023
View reviewed changes
Comment thread doc/api/events.md Resolved

Uh oh!

There was an error while loading. Please reload this page.

@deokjinkim
move sentence describing status of the WHATWG spec to legacy banner
daef80c
aduh95
aduh95 reviewed Jan 16, 2023
View reviewed changes
Comment thread doc/api/events.md Resolved

Uh oh!

There was an error while loading. Please reload this page.

@deokjinkim @aduh95
fix legacy banner by adding > symbol
c951aea
Co-authored-by: Antoine du Hamel <[email protected]>
aduh95
aduh95 reviewed Jan 16, 2023
View reviewed changes
Comment thread doc/api/events.md Resolved

Uh oh!

There was an error while loading. Please reload this page.

@deokjinkim @aduh95
fix options by adding nested option
0db45a6
Co-authored-by: Antoine du Hamel <[email protected]>
aduh95
aduh95 approved these changes Jan 16, 2023
View reviewed changes
@aduh95 aduh95 added request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. commit-queue-squash PRs the Commit Queue should land as one squashed commit. labels Jan 16, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jan 16, 2023
@nodejs-github-bot

nodejs-github-bot commented Jan 16, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/48995/

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

This was referenced Jan 17, 2023
CI Reliability 2023-01-17 nodejs/reliability#486
Open
CI Reliability 2023-01-18 nodejs/reliability#487
Open
@lpinca lpinca added the commit-queue PRs queued for automated landing through the Commit Queue. label Jan 19, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Jan 19, 2023
@nodejs-github-bot
nodejs-github-bot merged commit 2ff8c50 into nodejs:main Jan 19, 2023
@nodejs-github-bot

nodejs-github-bot commented Jan 19, 2023

Copy link
Copy Markdown
Collaborator

Landed in 2ff8c50

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

RafaelGSS pushed a commit that referenced this pull request Jan 20, 2023
@deokjinkim @RafaelGSS
events: add initEvent to Event
f85a8e4
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]>
@RafaelGSS RafaelGSS mentioned this pull request Jan 20, 2023
v19.5.0 proposal #46286
Merged
@juanarbol juanarbol added the backport-requested-v18.x label Jan 26, 2023
@juanarbol

juanarbol commented Jan 26, 2023

Copy link
Copy Markdown
Member

This is not landing cleanly in v18.x

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

Reviewers

@jasnell jasnell jasnell left review comments
@mcollina mcollina mcollina approved these changes
@benjamingr benjamingr benjamingr approved these changes
@anonrig anonrig anonrig approved these changes
@ronag ronag ronag approved these changes
@himself65 himself65 himself65 approved these changes
@aduh95 aduh95 aduh95 approved these changes
@RafaelGSS RafaelGSS RafaelGSS approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. commit-queue-squash PRs the Commit Queue should land as one squashed commit.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

13 participants

@deokjinkim @anonrig @benjamingr @annevk @nodejs-github-bot @aduh95 @juanarbol @mcollina @jasnell @ronag @himself65 @RafaelGSS @lpinca
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.