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

Jump to bottom

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

Merged
merged 1 commit into from Jan 7, 2023
Merged

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

merged 1 commit into from Jan 7, 2023

Conversation

deokjinkim
Copy link
Contributor

@deokjinkim deokjinkim commented Jan 4, 2023 •
edited

event.srcElement is described as legacy in spec.

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

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

@RaisinTen RaisinTen left a comment •
edited

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.

@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
Copy link
Contributor Author

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

@RaisinTen 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
RaisinTen approved these changes Jan 4, 2023
View reviewed changes
Copy link
Contributor

@RaisinTen RaisinTen left a 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

@RaisinTen
Copy link
Contributor

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

@aduh95
Copy link
Contributor

aduh95 commented Jan 4, 2023

Probably Legacy would be a better status, since we nor the web spec plans on ever actually removing it (I think)

@deokjinkim deokjinkim force-pushed the 230104_event_remove_srcelement branch from 1b33fbc to 4f8963d Compare January 5, 2023 00:20
@deokjinkim
Copy link
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
benjamingr requested changes Jan 6, 2023
View reviewed changes
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.

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

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

aduh95
aduh95 reviewed Jan 6, 2023
View reviewed changes
doc/api/deprecations.md Outdated Show resolved Hide resolved
doc/api/deprecations.md Outdated Show resolved Hide resolved
doc/api/events.md Outdated Show resolved Hide resolved
doc/api/events.md Outdated Show resolved Hide resolved
@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 deokjinkim force-pushed the 230104_event_remove_srcelement branch from b933185 to 834c2b3 Compare January 7, 2023 01:50
@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 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 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 7, 2023
@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 7, 2023
@nodejs-github-bot 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 nodejs-github-bot merged commit 513c151 into nodejs:main Jan 7, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 513c151

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 <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
@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 <raisinten@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Jan 20, 2023
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 <raisinten@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@juanarbol juanarbol mentioned this pull request Jan 28, 2023
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 <raisinten@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
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

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, 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.

None yet
5 participants
@deokjinkim @RaisinTen @aduh95 @benjamingr @nodejs-github-bot