lib,events: replace the attr listener of the wrapped listener with symbol. #36558

peng-huang-ch · 2020-12-18T04:14:51Z

Fixes: #36522

Replace the attr listener of the wrapped listener with a symbol.

Related Issues

Fixes: #36522

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

yashLadha

PTAL

yashLadha · 2020-12-18T05:57:02Z

lib/events.js

+  kEventListener: {
+    value: kEventListener,
+    enumerable: false,
+    configurable: false,
+    writable: false,


Why is this property needs to be attached to the ``EventEmitter` object?

Seem EventTarget needs to use the attributes, should keep the attr same, maybe could use common define, where do you think I can define it.

yashLadha · 2020-12-18T05:58:22Z

lib/events.js

+      const l = listener[kEventListener] ? listener[kEventListener] : listener;
+      target.emit('newListener', type, l);


l is not a very verbose name and doesn't dictates the meaning.

Fixed. remove the useless temporary variables.

yashLadha · 2020-12-18T05:59:23Z

lib/events.js

+    let handleListener = handler ? handler[kEventListener] : undefined;
+    while (handleListener !== undefined) {
+      ArrayPrototypePush(listeners, handleListener);


Why the need for removing null check?

Fixed, still use the optional chaining

yashLadha · 2020-12-18T06:14:20Z

test/parallel/test-event-emitter-listeners.js

@@ -24,6 +24,7 @@
 require('../common');
 const assert = require('assert');
 const events = require('events');
+const { kEventListener } = events;


Why is this required, can it be restructured?

Test the wrapped listener has this attr, and the value is the origin listener.

lpinca · 2020-12-18T07:31:05Z

I've added the semver-major label as the property is used by user land modules.

nodejs-github-bot · 2020-12-18T12:35:35Z

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

Fixes: #36522

Trott · 2020-12-24T16:59:28Z

@nodejs/events

jasnell

We should not remove the existing listener property without a proper deprecation cycle. It could be made an alias of the new symbol. Also, the symbol should be declared directly in events.js rather than in internal/util requiring a new import here. It would also likely be a good idea to use Symbol.for() (SymbolFor from primordials) to define it.

peng-huang-ch · 2021-01-02T14:03:54Z

@jasnell I have some questions:

If using SymbolFor the userland still could change the attribute, we should not allow do it.
The EventTarget also needs to use the attributes, so I defined the symbol in the internal/util. if declare directly in the event.js, then should export the attribute. same with above 1, will allow the userland to change the warped listener.

aduh95 · 2021-09-15T16:47:03Z

If using SymbolFor the userland still could change the attribute, we should not allow do it.

I think the goal is to let it be accessible to users, so they can mutate its value, but you decrease the risk of a clash with a user-specific utility (I mean, if someone is setting fn[Symbol.for('nodejs.listener')], it's almost certain they meant to deal with this API only). Another way of doing this would be indeed to export the symbol, in which case there's no need to use SymbolFor.
If you think we should instead remove this feature, it would need to go through a full deprecation cycle first.

peng-huang-ch · 2021-09-28T03:33:19Z

@aduh95 thanks for your reply.
I still think we shouldn't actually affect user-specific utilities. if two listeners have some relationship, it would be better to define it on the utility level.
If you disagree with it, maybe we should add some description for the event once to let the caller knows the impact on the registered listener.

aduh95 · 2021-09-28T08:21:51Z

I don't disagree on the philosophical level, the issue is it's being used by user land modules, so removing it wouldn't be acceptable without going through a deprecation cycle first. Improving the documentation is of course always welcome.

peng-huang-ch · 2021-09-28T11:03:29Z

Sure, I agree. but it may require the module developers to decide whether the modification is necessary and how to deal with it.

nodejs-github-bot added the events Issues and PRs related to the events subsystem / EventEmitter. label Dec 18, 2020

peng-huang-ch mentioned this pull request Dec 18, 2020

[Bug] Remove the incorrect event listener. #36522

Open

yashLadha suggested changes Dec 18, 2020

View reviewed changes

lpinca added the semver-major PRs that contain breaking changes and should be released in the next major version. label Dec 18, 2020

PoojaDurgad added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 18, 2020

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 18, 2020

lib,events: replace the attr listener of wrapped listener with symbol.

a298ab4

Fixes: #36522

benjamingr added the needs-benchmark-ci PR that need a benchmark CI run. label Dec 24, 2020

jasnell requested changes Dec 31, 2020

View reviewed changes

guybedford force-pushed the master branch from dc5a5da to 8e46568 Compare March 29, 2021 21:33

Trott force-pushed the main branch from 2d76238 to ca3ed36 Compare November 12, 2022 01:49

peng-huang-ch closed this by deleting the head repository Feb 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib,events: replace the attr listener of the wrapped listener with symbol. #36558

lib,events: replace the attr listener of the wrapped listener with symbol. #36558

peng-huang-ch commented Dec 18, 2020

yashLadha left a comment

yashLadha Dec 18, 2020

peng-huang-ch Dec 18, 2020 •

edited

Loading

yashLadha Dec 18, 2020

peng-huang-ch Dec 18, 2020 •

edited

Loading

yashLadha Dec 18, 2020

peng-huang-ch Dec 18, 2020

yashLadha Dec 18, 2020

peng-huang-ch Dec 18, 2020

lpinca commented Dec 18, 2020

nodejs-github-bot commented Dec 18, 2020

Trott commented Dec 24, 2020

jasnell left a comment

peng-huang-ch commented Jan 2, 2021

aduh95 commented Sep 15, 2021

peng-huang-ch commented Sep 28, 2021

aduh95 commented Sep 28, 2021

peng-huang-ch commented Sep 28, 2021

		const l = listener[kEventListener] ? listener[kEventListener] : listener;
		target.emit('newListener', type, l);

lib,events: replace the attr listener of the wrapped listener with symbol. #36558

lib,events: replace the attr listener of the wrapped listener with symbol. #36558

Conversation

peng-huang-ch commented Dec 18, 2020

Related Issues

Checklist

yashLadha left a comment

Choose a reason for hiding this comment

yashLadha Dec 18, 2020

Choose a reason for hiding this comment

peng-huang-ch Dec 18, 2020 • edited Loading

Choose a reason for hiding this comment

yashLadha Dec 18, 2020

Choose a reason for hiding this comment

peng-huang-ch Dec 18, 2020 • edited Loading

Choose a reason for hiding this comment

yashLadha Dec 18, 2020

Choose a reason for hiding this comment

peng-huang-ch Dec 18, 2020

Choose a reason for hiding this comment

yashLadha Dec 18, 2020

Choose a reason for hiding this comment

peng-huang-ch Dec 18, 2020

Choose a reason for hiding this comment

lpinca commented Dec 18, 2020

nodejs-github-bot commented Dec 18, 2020

Trott commented Dec 24, 2020

jasnell left a comment

Choose a reason for hiding this comment

peng-huang-ch commented Jan 2, 2021

aduh95 commented Sep 15, 2021

peng-huang-ch commented Sep 28, 2021

aduh95 commented Sep 28, 2021

peng-huang-ch commented Sep 28, 2021

peng-huang-ch Dec 18, 2020 •

edited

Loading

peng-huang-ch Dec 18, 2020 •

edited

Loading