-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix(nestjs): Add support for Symbol as event name #17785
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
fix(nestjs): Add support for Symbol as event name #17785
Conversation
The @onevent decorator accepts the following types for its argument: ```typescript string | symbol | Array<string | symbol> `` If a Symbol is included in an array, the code to get the eventName will throw a TypeError (String(event)) This occurs because JavaScript’s Array.prototype.join internally calls ToString on each array element. Per the specification, ToString(Symbol) is not allowed and results in a TypeError. To avoid this issue, do not rely on String(array) or .join() on arrays containing symbols directly. Instead, explicitly convert each element to a string while handling symbols safely. doc: https://tc39.es/ecma262/multipage/indexed-collections.html#sec-array.prototype.join doc: https://tc39.es/ecma262/multipage/abstract-operations.html#sec-tostring
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.
Bug: Symbol Handling Fails in Event Name Extraction
The eventNameFromEvent
function at line 82 uses String(event)
in its fallback, which throws a TypeError
when the event is a Symbol
. This prevents proper Symbol handling, contrary to the PR's intent.
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.
This should work: https://tc39.es/ecma262/multipage/text-processing.html#sec-string-constructor-string-value, but let me know
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.
Makes sense to me, thanks for contributing this!
Uh oh!
There was an error while loading. Please reload this page.
The
@OnEvent
decorator accepts the following types for its argument:If a Symbol is included in an array, the code to get the eventName will throw a TypeError (String(event)) This occurs because JavaScript’s
Array.prototype.join internally calls ToString on each array element. Per the specification, ToString(Symbol) is not allowed and results in a TypeError.
To avoid this issue, do not rely on String(array) or .join() on arrays containing symbols directly. Instead, explicitly convert each element to a string while handling symbols safely.
I couldn't find a way to test adding multiple
@OnEvent
so the second part can be tested. It didn't have any tests before (as far as I can tell), but would be nice to add them.doc: https://tc39.es/ecma262/multipage/indexed-collections.html#sec-array.prototype.join
doc: https://tc39.es/ecma262/multipage/abstract-operations.html#sec-tostring