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

fix(ext/dom_exception): align better with spec #15097

Merged
merged 9 commits into from Jul 20, 2022
Merged

fix(ext/dom_exception): align better with spec #15097

merged 9 commits into from Jul 20, 2022

Conversation

ghost
Copy link

@ghost ghost commented Jul 6, 2022

Fixes #12927.
There were no issues open about the current implementation.

Uses symbolic members for DOMException, and applies branding to its getters.
Uses a null prototype for the error string->code mapping object, to avoid prototype access.
Also updates outdated specification links.

@ry ry requested a review from lucacasonato July 7, 2022 15:28
Copy link
Member

@crowlKats crowlKats left a comment

Choose a reason for hiding this comment

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

I am very unsure about this usage of __proto__, but it does seem to work. everything else looks good

Copy link
Member

@lucacasonato lucacasonato left a comment

Choose a reason for hiding this comment

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

Let's not use __proto__.

The same effect can be achieved using Object.create.

@ghost
Copy link
Author

ghost commented Jul 19, 2022

I am very unsure about this usage of __proto__, but it does seem to work. everything else looks good

This __proto__ is a non-accessor object prototype initialization, and is the most optimised way to set it in all engines; see: tc39/ecma262#2125.

@ghost
Copy link
Author

ghost commented Jul 19, 2022

Let's not use __proto__.

The same effect can be achieved using Object.create.

Interestingly, this was how it was originally written, but was undone...
I'll try to leave a note about why it'll null too.

Reverted in c75f92c

@lucacasonato
Copy link
Member

@phosra Thanks.

This is not a performance sensitive path (it runs once during build of the Deno binary). As such we much prefer things like Object.create because it more clearly communicates the intent.

__proto__ is a code smell, just like complex Array.prototype.reduce operations. These are code patterns we'd like to avoid.

@ghost
Copy link
Author

ghost commented Jul 19, 2022

Acknowledged, fair enough.

Copy link
Member

@crowlKats crowlKats left a comment

Choose a reason for hiding this comment

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

LGTM now

@crowlKats crowlKats dismissed lucacasonato’s stale review July 20, 2022 07:20

issue has been addressed

@crowlKats crowlKats merged commit b8e1250 into denoland:main Jul 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DOMException's name->code mapping object observably accesses userland mutable Object.prototype
2 participants