Constructor & prototype reform#2691
Conversation
package.json
Outdated
| "watchify": "^3.11.1", | ||
| "wd": "^1.11.2", | ||
| "webidl2js": "^10.0.0" | ||
| "webidl2js": "https://github.com/pmdartus/webidl2js.git#pmdartus/constructor-reform-pt2" |
There was a problem hiding this comment.
Using the WIP version to run the test suite on the CI.
| // That is why we assign everything inside of the constructor, instead of using a shared prototype. | ||
| // You can verify this in e.g. Firefox or Internet Explorer, which do a good job with Web IDL compliance. | ||
|
|
||
| function Window(options) { |
There was a problem hiding this comment.
Hmm, the weirdness where this is named Window but the actual Window constructor is created on line 47 is strange... I'm not sure what the right architecture is. I guess we can try to straighten it out later, or just wait until we move Window to webidl2js.
There was a problem hiding this comment.
I wanted to minimize the amount of code change made to lib/jsdom/browser/Window.js, a clean up is overdue. Adding support for [Global] and [NamedConstructor] would greatly help. Let's do that in another PR.
lib/jsdom/living/interfaces.js
Outdated
| const xpath = require("../level3/xpath"); | ||
| const nodeFilter = require("./node-filter"); | ||
|
|
||
| const GENERATED_INTERFACES = [ |
There was a problem hiding this comment.
But isn’t it a common convention that constants are in UPPER_SNAKE_CASE?
lib/jsdom/living/xhr-utils.js
Outdated
| if (properties.uploadListener) { | ||
| fireAnEvent(event, xhr.upload, ProgressEvent, { loaded: 0, total: 0, lengthComputable: false }); | ||
| fireAnEvent("loadend", xhr.upload, ProgressEvent, { loaded: 0, total: 0, lengthComputable: false }); | ||
| const uploadWrapper = idlUtils.tryImplForWrapper(xhr.upload); |
There was a problem hiding this comment.
The name seems wrong here; should it be uploadImpl? In general not sure why this changed; was it a bug you found?
There was a problem hiding this comment.
Currently, XMLHttpRequest doesn't uses wedidl2js. All the properties including upload are wrappers and not impls. With this PR the fireAnEvent function expects an impl and not a wrapper, so I need to unwrap before the invocation.
I initially undertook the task of converting the XMLHttpRequest interface to leverage webidl2js, however it was too much change to fit into a single PR. Once this PR is merged, I need to open couple of issues to remove some of the hacks now that all the interfaces have access to the global object.
There was a problem hiding this comment.
That makes sense, I'm just confused why the variable name is uploadWrapper if you're calling (try)ImplForWrapper, which returns an impl.
There was a problem hiding this comment.
It should probably be renamed to uploadImpl or something.
lib/jsdom/living/xhr-utils.js
Outdated
| const uploadWrapper = idlUtils.implForWrapper(xhr.upload); | ||
| fireAnEvent(event, uploadWrapper, ProgressEvent, { loaded: 0, total: 0, lengthComputable: false }); | ||
| fireAnEvent("loadend", uploadWrapper, ProgressEvent, { loaded: 0, total: 0, lengthComputable: false }); |
There was a problem hiding this comment.
From #2691 (comment):
It should probably be renamed to
uploadImplor something.
| const uploadWrapper = idlUtils.implForWrapper(xhr.upload); | |
| fireAnEvent(event, uploadWrapper, ProgressEvent, { loaded: 0, total: 0, lengthComputable: false }); | |
| fireAnEvent("loadend", uploadWrapper, ProgressEvent, { loaded: 0, total: 0, lengthComputable: false }); | |
| const uploadImpl = idlUtils.implForWrapper(xhr.upload); | |
| fireAnEvent(event, uploadImpl, ProgressEvent, { loaded: 0, total: 0, lengthComputable: false }); | |
| fireAnEvent("loadend", uploadImpl, ProgressEvent, { loaded: 0, total: 0, lengthComputable: false }); |
domenic
left a comment
There was a problem hiding this comment.
OK sweet, this LGTM; we can merge once we get the webidl2js work merged.
I assume the plan of record for DOMException/URL/URLSearchParams/XMLSerializer is to do them as you did in this PR for now, consuming their old-webidl2js versions and adding to window directly. Then we can work on updating them later.
54fa585 to
ff5e3c5
Compare
This commit makes 21 tests fails, for 2 reasons: * Invalid construtor property - this is expected. * WebSocket [WebIDL2JSFactory] ext attr removal - some of the test mutates the prototype, and impacting the rest of the test suite.
ff5e3c5 to
618d57e
Compare
|
Yes, you are right the plan is to migrate the dependant projects as we go. I already opened a PR on domexception, I will try to update the other one soon. |
* allow observing elements within iframes (fixes #98) * upgrade jsdom; needed to get fix from jsdom/jsdom#2691 * test that observe can be called with Element created in an iframe * upgrade node on CircleCI to work with new jsdom
Changes
This PR applies the webdidl2js constructor & prototype reform to jsdom.
Fixes #1453 #2687