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.

Sign up for GitHub

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

Jump to bottom

feat: Throw DomException with name AbortError #482

Merged
merged 10 commits into from
Jan 13, 2023
Merged

feat: Throw DomException with name AbortError #482

merged 10 commits into from
Jan 13, 2023

Conversation

cristobal
Copy link
Contributor

Description

When throwing DomException it should provide AbortError name to the DomException constructor as specified in the MDN docs here

Screenshot 2023-01-11 at 15 49 58

The PR also includes fallback to use of Error class where the name property is overridden.

This PR addresses and closes the following issue #470

@cristobal
Copy link
Contributor Author

cristobal commented Jan 11, 2023 •
edited
Loading

The test get stuck for some reason, even when i run the tests locally on my Mac with latest node and from the main branch the tests are pending to finish.

Screenshot 2023-01-11 at 18 07 45

@cristobal
Copy link
Contributor Author

cristobal commented Jan 11, 2023 •
edited
Loading

Hmm tests run fine on node 14 on my Mac for both the main branch and this PR.
Not sure why the CI run failed then...

Screenshot 2023-01-11 at 18 12 55

@cristobal
Copy link
Contributor Author

cristobal commented Jan 11, 2023 •
edited
Loading

Seems that the pending tests do not work on the latest node 14.21.1, when running with node 14.19.3 as in the previous comment the tests work fine. However when running with 14.21.1 as in the picture below:
Screenshot 2023-01-11 at 19 39 45

Which is the same version to run the github workflow that failed, will try to create a separate PR that addresses this issue 👋🏽

sindresorhus
sindresorhus requested changes Jan 12, 2023
View reviewed changes
test/main.ts Outdated Show resolved Hide resolved
source/errors/DOMException.ts Outdated Show resolved Hide resolved
source/errors/DOMException.ts Outdated Show resolved Hide resolved
test/main.ts Outdated Show resolved Hide resolved
cristobal and others added 5 commits January 12, 2023 08:21
@cristobal @sindresorhus
Update test/main.ts
918a27a 
Co-authored-by: Sindre Sorhus <sindresorhus@gmail.com>
@cristobal @sindresorhus
Update source/errors/DOMException.ts
7e2d2c0 
Co-authored-by: Sindre Sorhus <sindresorhus@gmail.com>
@cristobal @sindresorhus
Update source/errors/DOMException.ts
79cb34e 
Co-authored-by: Sindre Sorhus <sindresorhus@gmail.com>
@cristobal
use isDomExceptionSupported
b673733
@cristobal
use t.is
cb47245
@cristobal
Copy link
Contributor Author

@sindresorhus thanks for the feedback the requested changes have been addressed, also created a separate PR here, that addresses the flaky browsers tests 👋🏽

@sindresorhus
Copy link
Owner

A test is failing on Node.js 18

@cristobal
Copy link
Contributor Author

A test is failing on Node.js 18

Weird looking into to 👋🏽

cristobal
cristobal commented Jan 13, 2023
View reviewed changes
test/main.ts Outdated Show resolved Hide resolved
@cristobal
Copy link
Contributor Author

@sindresorhus fixed was a typo in the test name should be DOMException not DomException my bad🤦🏽

@sindresorhus sindresorhus merged commit ce8588d into sindresorhus:main Jan 13, 2023
@cristobal cristobal deleted the dom-exception-with-name-AbortError branch January 13, 2023 08:38
@sindresorhus sindresorhus mentioned this pull request Jan 13, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sindresorhus sindresorhus Awaiting requested review from sindresorhus

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@cristobal @sindresorhus