Remove dependance on global window and document

[KoryNunn]

Use window and document from render target instead of using globals.
This makes unit and intergration testing much easier.

Description

$window and $doc get set when render is called, and are based on the passed dom

Motivation and Context

This decouples mithril from its environment and instead only couples it to DOM.
This allows tests environments to be more easily established, making testing in Node etc easier.
This also allows tests to run in parallel, which, while not needed for mithril it's self, can be useful in larger applications that use mithril.

How Has This Been Tested?

All the existing tests pass

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation change

Checklist:

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• All new and existing tests passed.
• I have updated docs/changelog.md

Happy to check the last points off above if this PR is likely to get merged.

[dead-claudia]

I've got a better idea:

1. Replace $doc with elem.parentDocument everywhere and just drop the otherwise unused $window.
2. Remove the closure mithril/render/render currently uses and just export the returned render function directly.

This comes with some added benefits:

1. No need for $doc.defaultView
2. You can render inside iframe windows from outside them and it'll select the right window to render to.

[dead-claudia]

See reply

[KoryNunn]

Yep sounds great, I'd suggest doing it in stages as changing the export is a breaking change for anyone who uses render directly ( like me :) )

IMO:

1. Get this in as it's completely non-breaking, and a step in the right direction/intent.
2. PR/Test using parentDocument, I'm not familiar enough with the codebase to be certain that it's non-breaking
3. Propose/PR unwrapping render.

[KoryNunn]

I had a look at only using parent.ownerDocument but that's not viable since DocumentFragment doesn't have an owner document.

Best case we could use parent.ownerDocument when it's available and fall back to the top-level render target for when parent is a DocumentFragment.

This (and my PR) does still leave an issue where render would no longer be able to target DocumentFragments. Maybe a solution to this is to keep the code as is, but use the target node if no document is passed in.

[KoryNunn]

I've updated to implement the last solution I proposed.

This adds a number of fn calls that may have a slight perf impact, we could inline if required.

[dead-claudia]

I had a look at only using parent.ownerDocument but that's not viable since DocumentFragment doesn't have an owner document.

Any browser that returns null for ownerDocument for document fragments is violating the spec: https://dom.spec.whatwg.org/#dom-node-ownerdocument

And this has been the case since DOM Level 1, back in 1998: https://www.w3.org/TR/1998/REC-DOM-Level-1-19981001/level-one-core.html#ID-1950641247

DOM Level 2 created one other exception, DocumentType nodes, but that exception has since been removed from the living standard (it's impossible in practice to create one without a document anyways).

Edit: grammar, clarity.

[dead-claudia]

@KoryNunn Forgot to ping. Also, WPT tests this for the new DocumentFragment() constructor variant: https://github.com/web-platform-tests/wpt/blob/0a9f334462/dom/nodes/DocumentFragment-constructor.html#L13

It's difficult to imagine how new DocumentFragment() would differ from document.createDocumentFragment(). (And to be honest, it's surprising this isn't tested for directly.)

You can safely it exists for all nodes with a nodeType other than Node.DOCUMENT_NODE === 9 or Node.DOCUMENT_TYPE_NODE === 10. And of these, documents forward element insertions to their documentElement and doctypes don't accept children at all.

If you truly want something bulletproof, you can use this code at the top of the render function and then proceed to use the current node's ownerDocument in place of $doc everywhere:

// The condition is equivalent to the following, but saves some space and is slightly faster:
// `root.nodeType !== 1 && root.nodeType !== 10`
if (~0x402 >> root.nodeType & 1) {
    // Set the real root to an element if a document node
    if (root.nodeType === 9) {
        root = root.documentElement
    } else {
        throw new TypeError("Root must be a document, an element, or a document fragment")
    }
}

Edit: note to self and proofread before sending. (Missed some words.)

[KoryNunn]

Yep I was pretty surprised that DocumentFragment didn't have ownerDocument, and it turns out I just forgot to add it to the domMock for DocumentFragment

PR now only uses parent.ownerDocument
All tests passing.

Closure still exists as there are other variables that seem necessary .

Edit: I'd personally just say direct rendering into document should be frowned upon :)

[dead-claudia]

Okay, minor nit: can you move the getDocument call all the activeElement calls accept into activeElement itself? Otherwise looks fine.

[dead-claudia]

Also, can you run the benchmark suite before and after this patch and share the results here? I expect this to have minor impact on create but to be insignificant elsewhere, and I'd like to have some validation here.

[KoryNunn]

Updated.

Perf results:

Before:

> [email protected] perf
> node performance/test-perf.js

construct large vnode tree x 41,669 ops/sec ±0.81% (131 runs sampled)
rerender identical vnode x 11,902,057 ops/sec ±2.00% (122 runs sampled)
rerender same tree x 97,702 ops/sec ±1.11% (127 runs sampled)
add large nested tree x 12,689 ops/sec ±1.06% (124 runs sampled)
mutate styles/properties x 151 ops/sec ±1.60% (105 runs sampled)
repeated add/removal x 5,487 ops/sec ±2.00% (123 runs sampled)
Completed perf tests in 49874ms

After:

> [email protected] perf
> node performance/test-perf.js

construct large vnode tree x 40,003 ops/sec ±1.66% (128 runs sampled)
rerender identical vnode x 11,305,752 ops/sec ±0.95% (131 runs sampled)
rerender same tree x 101,118 ops/sec ±1.40% (116 runs sampled)
add large nested tree x 13,239 ops/sec ±1.09% (126 runs sampled)
mutate styles/properties x 150 ops/sec ±2.36% (104 runs sampled)
repeated add/removal x 5,820 ops/sec ±2.19% (118 runs sampled)
Completed perf tests in 49948ms

Looks like negligible difference.
FYI: this is running on an i5 laptop in WSL.