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

lib: make lazyDOMException more common #39105

Closed
wants to merge 2 commits into from

Conversation

XadillaX
Copy link
Contributor

No description provided.

@github-actions github-actions bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Jun 21, 2021
lib/buffer.js Outdated Show resolved Hide resolved
Comment on lines +447 to +448
if (DOMException === undefined)
DOMException = internalBinding('messaging').DOMException;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (DOMException === undefined)
DOMException = internalBinding('messaging').DOMException;
DOMException ??= internalBinding('messaging').DOMException;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why should we use ??=?

Copy link
Contributor

@aduh95 aduh95 Jun 21, 2021

Choose a reason for hiding this comment

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

I find ??= more readable and concise than the if statement, feel free to disagree and/or disregard.

let DOMException;
const lazyDOMException = hideStackFrames((message, name) => {
if (DOMException === undefined)
DOMException = internalBinding('messaging').DOMException;
Copy link
Member

Choose a reason for hiding this comment

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

I think it's good to hoist this out of the crypto util and make it more universal in the code base, but I am wondering, why do we want to make this lazy?

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably to avoid building the JS function until we're on the error path?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@XadillaX
Copy link
Contributor Author

Landed in 2de139b

XadillaX added a commit that referenced this pull request Jun 28, 2021
PR-URL: #39105
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@XadillaX XadillaX closed this Jun 28, 2021
targos pushed a commit that referenced this pull request Jul 11, 2021
PR-URL: #39105
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
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants