Conversation
lib/public-api.js
Outdated
| function installDOMException(globalObject) { | ||
| DOMException.install(globalObject); | ||
|
|
||
| if (!globalObject.Error || typeof globalObject.Error !== "function") { |
There was a problem hiding this comment.
When attempting to integrate this change in jsdom, I found out that the window object doesn't expose the primordials and the following branch check fails. I also realized that some built-ins (like ArrayBuffer) are attached but there is a TODO to remove them which is strange.
IMO, we should remove the comment and expose all the built-ins. Thoughts?
There was a problem hiding this comment.
Hmm hmm. So what's going on here is that if you don't enable scripting in jsdom, then none of the ECMAScript globals get installed. I guess we alias some of them to the outer globals anyway to make some tests pass. If you do enable scripting, then they should all be installed by the vm module.
I think what we should do is either:
-
Remove support for no-scripting from jsdom. This ensures that consistently all jsdom windows have non-shared globals, no matter whether those are web globals or ECMAScript globals.
-
Change our strategy to branch on scripting support. If scripting is not enabled, then alias all the ECMAScript globals (not just the small list we do today). If it is enabled, then alias none of them.
The drawback of the first option here is the potential performance hit for the cases where people don't need to run scripting. The drawback of the second is inconsistency.
domenic
left a comment
There was a problem hiding this comment.
I'd like to take a slightly different approach here, which is to have two modules, e.g. index.js and webidl2js-wrapper.js. Then any existing consumers who just want a shared-global instance would use require("domexception"), and jsdom would do require("domexception/webidl2js-wrapper"). This makes it simpler for basic consumers, and also avoids creating a redundant shared-global class for jsdom (as long as jsdom never requires index.js).
lib/public-api.js
Outdated
| function installDOMException(globalObject) { | ||
| DOMException.install(globalObject); | ||
|
|
||
| if (!globalObject.Error || typeof globalObject.Error !== "function") { |
There was a problem hiding this comment.
Hmm hmm. So what's going on here is that if you don't enable scripting in jsdom, then none of the ECMAScript globals get installed. I guess we alias some of them to the outer globals anyway to make some tests pass. If you do enable scripting, then they should all be installed by the vm module.
I think what we should do is either:
-
Remove support for no-scripting from jsdom. This ensures that consistently all jsdom windows have non-shared globals, no matter whether those are web globals or ECMAScript globals.
-
Change our strategy to branch on scripting support. If scripting is not enabled, then alias all the ECMAScript globals (not just the small list we do today). If it is enabled, then alias none of them.
The drawback of the first option here is the potential performance hit for the cases where people don't need to run scripting. The drawback of the second is inconsistency.
lib/DOMException-impl.js
Outdated
| } | ||
| }; | ||
|
|
||
| // Special install function to make the DOMException inherit from Error. |
There was a problem hiding this comment.
The circular dependency here seems pretty weird, so I'd rather move this into webidl2js-wrapper.js. (Then maybe delete the inserted whitespace lines so that the diff is just globalObject,
Changes
11.0.0.DOMExceptionandwebidl2jsDOMException. This is a breaking change since this package used to eposeDOMExceptionas a default export.