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

use DOMException #49

Closed
wants to merge 2 commits into from
Closed

use DOMException #49

wants to merge 2 commits into from

Conversation

Quelklef
Copy link

@Quelklef Quelklef commented Jun 21, 2020

This ended up being harder than anticipated, mostly due to tooling.

For the most part, the change was simple. In errors.ts, I've replace class SomeError extends Error with function newSomeError(...): DOMException, and updated all callers.

However, there are two regressions and broken one tool, and I don't quite know how to fix them.

  1. Regression one is that the w3c test in src/test/web-platform-tests/converted/error-attributes.js now fails (line 218).

  2. (now fixed) Regression two is that the Mocha test should allow index where not all records have keys now fails

  3. Issue three is that qunit now fails altogether; this seems to be caused by the new domexception dependency using object spread notation on line 15 of webidl2js-wrapper.js.

Also, I had to do something a little hacky: after bashing my head on the wall trying to get TypeScript to recognize that the imported DOMException had the type of native DOMException, I eventually just did

global['DOMException'] = require('domexception'); // ts:ignore-line

in order to sidestep both TypeScript and tslint.

I'm not sure how to resolve these issues, since I'm unfamiliar with the codebase and have never used phantomjs or qunit. I tried to look into the first two, but didn't really get very far. Any suggestions?

@Quelklef Quelklef changed the title yet) use DOMException use DOMException Jun 21, 2020
This fixes the Mocha test regression
@jimmywarting
Copy link

NodeJS dose actually have DOMException built in right now (think it was mostly due to wanting to have fetch built in)

it's exposed globally as of v17.0.0
but it landed much more earlier than that (in v10.5 in fact) but required a round about way of getting it...
that's why i have also created https://www.npmjs.com/package/node-domexception to this end...

domenics DOMException is a complete custom copy of the DOMException and not the same instance as those you would catch an error from. so the instanceof checks are problematic.

my recommendations are, use either globalThis.DOMException (require node v17+) or use my package instead.

@dumbmatter
Copy link
Owner

I procrastinated on this long enough that now all the supported versions of Node.js have DOMException built in, which seems to work fine :)

@dumbmatter dumbmatter closed this May 20, 2024
@dumbmatter
Copy link
Owner

Also @jimmywarting your package is very clever! It's almost sad that it's no longer needed :)

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.

None yet

3 participants