FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

events: change status of `event.srcElement` to legacy by deokjinkim · Pull Request #46085 · nodejs/node · GitHub

/ node Public

events: change status of event.srcElement to legacy - #46085

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
deokjinkim:230104_event_remove_srcelement
Jan 7, 2023
ConversationCommits (1)ChecksFiles changed
Merged

events: change status of event.srcElement to legacy#46085
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
deokjinkim:230104_event_remove_srcelement

Conversation

deokjinkim commented Jan 4, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

event.srcElement is described as legacy in spec.

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

nodejs-github-bot added the needs-ci PRs that need a full CI run. label Jan 4, 2023
RaisinTen requested changes Jan 4, 2023
View reviewed changes

RaisinTen left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

deokjinkim force-pushed the 230104_event_remove_srcelement branch from 28415d3 to 1b33fbc Compare January 4, 2023 14:24
deokjinkim changed the title events: remove srcElement from Event events: deprecate event.srcElement Jan 4, 2023

deokjinkim commented Jan 4, 2023

Copy link
Copy Markdown
Contributor Author

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

RaisinTen added doc Issues and PRs related to the documentations. notable-change PRs with changes that should be highlighted in changelogs. deprecations Issues and PRs related to deprecations. events Issues and PRs related to the events subsystem / EventEmitter. labels Jan 4, 2023
RaisinTen approved these changes Jan 4, 2023
View reviewed changes

RaisinTen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM, @nodejs/events PTAL

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.

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)

deokjinkim force-pushed the 230104_event_remove_srcelement branch from 1b33fbc to 4f8963d Compare January 5, 2023 00:20

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.

benjamingr requested changes Jan 6, 2023
View reviewed changes

benjamingr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

benjamingr commented Jan 6, 2023

Copy link
Copy Markdown
Member

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

aduh95 reviewed Jan 6, 2023
View reviewed changes
Comment thread doc/api/deprecations.md Outdated
Comment thread doc/api/deprecations.md Outdated
Comment thread doc/api/events.md Outdated
Comment thread doc/api/events.md Outdated
deokjinkim force-pushed the 230104_event_remove_srcelement branch from 17494ff to b933185 Compare January 7, 2023 01:40
deokjinkim changed the title events: deprecate event.srcElement events: change status of event.srcElement to legacy Jan 7, 2023
deokjinkim force-pushed the 230104_event_remove_srcelement branch from b933185 to 834c2b3 Compare January 7, 2023 01:50
aduh95 requested a review from benjamingr January 7, 2023 08:53
aduh95 added eventtarget Issues and PRs related to the EventTarget implementation. and removed events Issues and PRs related to the events subsystem / EventEmitter. 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
aduh95 added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Jan 7, 2023
aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 7, 2023
nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 7, 2023
nodejs-github-bot merged commit 513c151 into nodejs:main Jan 7, 2023

nodejs-github-bot commented Jan 7, 2023

Copy link
Copy Markdown
Collaborator

Landed in 513c151

RafaelGSS pushed a commit to RafaelGSS/node that referenced this pull request Jan 17, 2023
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 <raisinten@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Jan 20, 2023
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 <raisinten@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
RafaelGSS mentioned this pull request Jan 20, 2023
Merged
juanarbol pushed a commit that referenced this pull request Jan 26, 2023
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 <raisinten@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
juanarbol mentioned this pull request Jan 28, 2023
Merged
juanarbol pushed a commit that referenced this pull request Jan 31, 2023
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 <raisinten@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

benjamingr benjamingr approved these changes
aduh95 aduh95 approved these changes
RaisinTen RaisinTen approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. doc Issues and PRs related to the documentations. 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.

5 participants

Back | FazBrowse Home | New Git URL