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: change status of `event.srcElement` to legacy by deokjinkim · Pull Request #46085 · nodejs/node

Conversation

@deokjinkim

@deokjinkim deokjinkim commented Jan 4, 2023 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

event.srcElement is described as legacy in spec.

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

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 4, 2023
RaisinTen
RaisinTen requested changes Jan 4, 2023
View reviewed changes

@RaisinTen RaisinTen 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.

It needs to start with a doc-deprecation, like - #41872.

Also, deprecation != deletion, so even if this gets deprecated, I don't think it has to be removed altogether unless there's a real need.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@deokjinkim
deokjinkim force-pushed the 230104_event_remove_srcelement branch from 28415d3 to 1b33fbc Compare January 4, 2023 14:24
@deokjinkim deokjinkim changed the title events: remove srcElement from Event events: deprecate event.srcElement Jan 4, 2023
@deokjinkim

deokjinkim commented Jan 4, 2023

Copy link
Copy Markdown
Contributor Author

@RaisinTen Thank you for kind explanation. Applied doc-deprecation instead of delete.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@RaisinTen RaisinTen added doc Issues and PRs related to Node.js documentation. notable-change PRs with changes that should be highlighted in changelogs. deprecations Issues and PRs related to deprecations. events Issues and PRs related to EventEmitter and the events module. labels Jan 4, 2023
RaisinTen
RaisinTen approved these changes Jan 4, 2023
View reviewed changes

@RaisinTen RaisinTen 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, @nodejs/events PTAL

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@RaisinTen

RaisinTen commented Jan 4, 2023

Copy link
Copy Markdown
Member

@deokjinkim could you also update the PR description? It still says that srcElement is being removed.
All reactions

Sorry, something went wrong.

Uh oh!

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

aduh95 commented Jan 4, 2023

Copy link
Copy Markdown
Contributor

Probably Legacy would be a better status, since we nor the web spec plans on ever actually removing it (I think)
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.
@deokjinkim
deokjinkim force-pushed the 230104_event_remove_srcelement branch from 1b33fbc to 4f8963d Compare January 5, 2023 00:20
@deokjinkim

deokjinkim commented Jan 5, 2023

Copy link
Copy Markdown
Contributor Author

@deokjinkim could you also update the PR description? It still says that srcElement is being removed.

Updated the PR description. Thank you for guide.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
benjamingr
benjamingr requested changes Jan 6, 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.

event.srcElement is a legacy feature (according to the spec).

It's just a filler for spec compliance in Node.js since we don't have any event targets that do event propagation.

I seriously doubt that it'd ever be removed so I don't think we should deprecate it which implies removal until the spec does.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@benjamingr

benjamingr commented Jan 6, 2023

Copy link
Copy Markdown
Member

(That said, thank you for working on this!)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
aduh95
aduh95 reviewed Jan 6, 2023
View reviewed changes
Comment thread doc/api/deprecations.md Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread doc/api/deprecations.md Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread doc/api/events.md Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread doc/api/events.md Resolved

Uh oh!

There was an error while loading. Please reload this page.
@deokjinkim
deokjinkim force-pushed the 230104_event_remove_srcelement branch from 17494ff to b933185 Compare January 7, 2023 01:40
@deokjinkim deokjinkim changed the title events: deprecate event.srcElement events: change status of event.srcElement to legacy Jan 7, 2023
@deokjinkim
events: change status of event.srcElement to legacy
834c2b3 
`event.srcElement` is described as legacy in spec.

Refs: https://dom.spec.whatwg.org/#interface-event
@deokjinkim
deokjinkim force-pushed the 230104_event_remove_srcelement branch from b933185 to 834c2b3 Compare January 7, 2023 01:50
aduh95
aduh95 approved these changes Jan 7, 2023
View reviewed changes
@aduh95
aduh95 requested a review from benjamingr January 7, 2023 08:53
@aduh95 aduh95 added eventtarget Issues and PRs related to the EventTarget implementation. and removed events Issues and PRs related to EventEmitter and the events module. notable-change PRs with changes that should be highlighted in changelogs. deprecations Issues and PRs related to deprecations. needs-ci PRs that need a full CI run. labels Jan 7, 2023
benjamingr
benjamingr approved these changes Jan 7, 2023
View reviewed changes
@aduh95 aduh95 added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Jan 7, 2023
@aduh95 aduh95 added the commit-queue PRs queued for automated landing through the Commit Queue. label Jan 7, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Jan 7, 2023
@nodejs-github-bot
nodejs-github-bot merged commit 513c151 into nodejs:main Jan 7, 2023
@nodejs-github-bot

nodejs-github-bot commented Jan 7, 2023

Copy link
Copy Markdown
Collaborator

Landed in 513c151
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
RafaelGSS pushed a commit to RafaelGSS/node that referenced this pull request Jan 17, 2023
@deokjinkim @RafaelGSS
events: change status of event.srcElement to legacy
f92dcc6 
`event.srcElement` is described as legacy in spec.

Refs: https://dom.spec.whatwg.org/#interface-event
PR-URL: nodejs#46085
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Jan 20, 2023
@deokjinkim @RafaelGSS
events: change status of event.srcElement to legacy
5304c89 
`event.srcElement` is described as legacy in spec.

Refs: https://dom.spec.whatwg.org/#interface-event
PR-URL: #46085
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[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 pushed a commit that referenced this pull request Jan 26, 2023
@deokjinkim @juanarbol
events: change status of event.srcElement to legacy
a47522c 
`event.srcElement` is described as legacy in spec.

Refs: https://dom.spec.whatwg.org/#interface-event
PR-URL: #46085
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
@juanarbol juanarbol mentioned this pull request Jan 28, 2023
V18.14.0 proposal #46396
Merged
juanarbol pushed a commit that referenced this pull request Jan 31, 2023
@deokjinkim @juanarbol
events: change status of event.srcElement to legacy
36be0c4 
`event.srcElement` is described as legacy in spec.

Refs: https://dom.spec.whatwg.org/#interface-event
PR-URL: #46085
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benjamingr benjamingr benjamingr approved these changes
@aduh95 aduh95 aduh95 approved these changes
@RaisinTen RaisinTen RaisinTen 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. doc Issues and PRs related to Node.js documentation. eventtarget Issues and PRs related to the EventTarget implementation.

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.

5 participants

@deokjinkim @RaisinTen @aduh95 @benjamingr @nodejs-github-bot
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.