Use new webidl2js bundle-entry.js feature #2092
Conversation
| @@ -1,5 +1,6 @@ | |||
| [Constructor, | |||
| Exposed=Window] | |||
| Exposed=Window, | |||
| LegacyWindowAlias=HTMLDocument] | |||
There was a problem hiding this comment.
Note that this might no longer be correct because of whatwg/html#4792.
There was a problem hiding this comment.
Well whatwg/dom#551 was closed, so this change should be reverted.
TimothyGu
force-pushed
the
expose-master
branch
from
ae7ce49
to
fc44269
Compare
|
This looks like a great improvement over the previous code. The comment you added makes me wonder, though...
Maybe we could take this as an opportunity to give these objects names that better clarify their purpose? While I'm familiar enough with the codebase to know what they are, having two different Perhaps the EDIT: ...seeing how many locations the |
|
@Zirro I wholeheartedly agree. I plan on coming back to this after getting #2088 merged (review welcome!). I'll also think of how to integrate some good parts from my TimothyGu/jsdom: |
|
So we're still sharing prototypes with this change, but have a proper window object in all class instances? |
|
No, this doesn’t change the status quo regarding that: all prototypes are shared, most classes are window-agnostic. It only simplifies the code as it is. The passed |
| } | ||
| this._core = dom; | ||
| // TODO move this somewhere else. |
There was a problem hiding this comment.
Running this on every window initialization seems sad since it's all a shared object.
| @@ -36,14 +33,14 @@ const WindowEventHandlersImpl = require("../living/nodes/WindowEventHandlers-imp | |||
| // NB: the require() must be after assigning `module.exports` because this require() is circular | |||
| // TODO: this above note might not even be true anymore... figure out the cycle and document it, or clean up. | |||
| module.exports = Window; | |||
| const dom = require("../living"); | |||
| const bootstrapDOM = require("../living/generated/bundle-entry").bootstrap; | |||
| const extraDOM = require("../living"); | |||
There was a problem hiding this comment.
I'd like us to carefully consider our names here. E.g. instead of "extraDOM", maybe "nonGeneratedInterfaces" or similar.
Similarly, instead of "bootstraDOM"... what? What does this function do? It's not clear to me.
| @@ -81,7 +90,7 @@ function Window(options) { | |||
|
|
|||
| // List options explicitly to be clear which are passed through | |||
| this._document = Document.create([], { | |||
| core: dom, | |||
| core: this._core, | |||
There was a problem hiding this comment.
Shouldn't be necessary after #2098.
| @@ -60,15 +57,27 @@ function Window(options) { | |||
| ///// INTERFACES FROM THE DOM | |||
| // TODO: consider a mode of some sort where these are not shared between all DOM instances | |||
| // It'd be very memory-expensive in most cases, though. | |||
| for (const name in dom) { | |||
| this._core = bootstrapDOM("Window", window, { window }); | |||
There was a problem hiding this comment.
We no longer store _core on Window after #2098. Not sure how that fits in here.
| @@ -355,6 +355,8 @@ const mappings = { | |||
| } | |||
| }; | |||
|
|
|||
| // N.B. the "core" here is the "extraDOM" object imported from living/index.js, not the core tied to a specific Window. | |||
| // Thus, it only contains the limited set of objects in living/index.js, but that is enough currently. | |||
| module.exports = core => { | |||
There was a problem hiding this comment.
Let's just update the parameter name then, instead of creating a comment explaining why it's misleading.
Uses the feature introduced in jsdom/webidl2js#101. Skipping CI for now as the change has not yet been merged, much less published, in webidl2js.
The result is a beauty.
[ci skip]