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: add cause to DOMException #44703

Merged

Conversation

flakey5
Copy link
Member

@flakey5 flakey5 commented Sep 17, 2022

Adds an optional cause field to DOMException as per the WebIDL standard added in whatwg/webidl#1179

@jasnell @panva

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Sep 17, 2022
@flakey5 flakey5 force-pushed the flakey5/add-domexception-cause branch from 400012e to c871867 Compare September 17, 2022 21:41
@flakey5 flakey5 force-pushed the flakey5/add-domexception-cause branch from ff4713f to a81a794 Compare September 18, 2022 00:41
@flakey5 flakey5 force-pushed the flakey5/add-domexception-cause branch from a81a794 to 7fc758b Compare September 18, 2022 16:20
@flakey5 flakey5 force-pushed the flakey5/add-domexception-cause branch from 3c85e9a to 004fb4c Compare September 22, 2022 00:43
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

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

Overall LGTM. Thank you for working on this!

It would be great if we can have a documentation update on this. Also, a note on the behavior divergence between the (pending) WebIDL definition would be helpful, like say, experimental.

@jasnell
Copy link
Member

jasnell commented Oct 2, 2022

While @flakey5 is working on remembering his password to log into GitHub from his phone ;-)... When we were discussing this change we noticed that the documentation for DOMException only links to MDN. It doesn't break down any of the API. I agree we should have docs but I'd prefer to do that in a separate PR that covers all of DOMException.

Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

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

Sounds good to me. Thanks.

@nodejs-github-bot
Copy link
Collaborator

@jasnell jasnell added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 4, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 4, 2022
@nodejs-github-bot nodejs-github-bot merged commit 00e5617 into nodejs:main Oct 4, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 00e5617

panva pushed a commit to panva/node that referenced this pull request Oct 4, 2022
PR-URL: nodejs#44703
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
danielleadams pushed a commit that referenced this pull request Oct 11, 2022
PR-URL: #44703
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants