[ShadyDOM] Add option for on-demand patching #189
Conversation
Can be used when code should be compatible with noPatch = true and noPatch = 'on-demand' for better performance for all APIs except: * `appendChild` and `insertBefore` (when the node is being moved from a location where it was logically positioned in the DOM) * when setting `className`/`class` * when calling `querySelector|All` * when setting `textContent` or `innerHTML`.
This is so big because every call to `wrap` was changed to `wrapIfNeeded` when possible.
Ensure `ShadyDOM.wrapIfNeeded` wraps when `on-demand` patching is used on browsers without descriptors since these browsers don't support on-demand patching.
packages/shadydom/src/link-nodes.js
Outdated
| * @param {AccessorType} type | ||
| */ | ||
| function patchNode(node, type) { | ||
| if (utils.settings.hasDescriptors && utils.settings.noPatch) { |
There was a problem hiding this comment.
Pull these out to const in the module since this is a hot path?
packages/shadydom/src/link-nodes.js
Outdated
| function patchNode(node, type) { | ||
| if (utils.settings.hasDescriptors && utils.settings.noPatch) { | ||
| patchNodeProto(node); | ||
| } { |
There was a problem hiding this comment.
Is this supposed to be } else { ??
packages/shadydom/src/link-nodes.js
Outdated
| * @enum {number} | ||
| */ | ||
| const AccessorType = { | ||
| inside: 1, |
There was a problem hiding this comment.
Are we sure these get inlined and not dereferenced?
| @@ -71,16 +71,66 @@ const getPatchPrototype = (name) => window[name] && window[name].prototype; | |||
| // CustomElements polyfill *only* cares about these accessors. | |||
| const disallowedNativePatches = utils.settings.hasDescriptors ? null : ['innerHTML', 'textContent']; | |||
|
|
|||
| /** | |||
| * Patch a group of accessors on an object only if it exists or if the `force` | |||
There was a problem hiding this comment.
force argument removed?
| } | ||
| } | ||
|
|
||
| const PROTO_IS_PATCHED = utils.SHADY_PREFIX + 'patchedProto'; | ||
|
|
||
| const patchedProtos = new Map(); |
There was a problem hiding this comment.
Add comment that this is making a pre-patched proto for TextNode, since any other instance patching is guaranteed to be an Element, that way patchNodeProto can assume it just needs to apply the element patches if there's not a pre-patched proto, eliminating a nodeType check.
There was a problem hiding this comment.
Also, let's add Comment, ProcessingInstruction, and CDATASection to the pre-patched list.
| return attachShadow(this, options); | ||
| } | ||
| const root = attachShadow(this, options); | ||
| // TODO(sorvell): Workaround for CE not seeing shadowRoot in augmented |
There was a problem hiding this comment.
"augmented" means "on-demand"? Why wasn't this a problem in regular noPatch mode?
There was a problem hiding this comment.
Only in on-demand patching mode since we're only patching here. Added better comment.
| // noPatch mode. CE's attachShadow patch is overwritten by this patch | ||
| // and cannot set its own special tracking for shadowRoot. It does this | ||
| // to be able to see closed shadowRoots. | ||
| this['__CE_shadowRoot'] = root; |
There was a problem hiding this comment.
CE polyfill also does internals.patchNode(shadowRoot); in its attachShadow patch -- this isn't needed in noPatch/on-demand because ShadyDOM's innerHTML/textContent instance patches would cause CE reactions via removeChild/appendChild?
packages/shadydom/src/shadydom.js
Outdated
| // `getRootNode`, `isConnected`, `slot`, `assignedSlot`, `assignedNodes`. | ||
| 'wrapIfNeeded': utils.settings.noPatch === true || | ||
| (utils.settings.noPatch === 'on-demand' && !utils.settings.hasDescriptors) ? | ||
| wrap : patch, |
There was a problem hiding this comment.
Let's review the logic for what wrap and wrapIfNeeded get set to together (and cross-check against Polymer's versions)...
packages/shadydom/src/shadydom.js
Outdated
| @@ -121,6 +138,9 @@ if (utils.settings.inUse) { | |||
| applyPatches(); | |||
| // Patch click event behavior only if we're patching | |||
| patchClick() | |||
| } else if (utils.settings.patchOnDemand) { | |||
| // in noPatch, do patch `attachShadow` and `shadowRoot` | |||
There was a problem hiding this comment.
| // in noPatch, do patch `attachShadow` and `shadowRoot` | |
| // in "on-demand" patching, do patch `attachShadow` and `shadowRoot`. This | |
| // unconditional patch is what kicks off all other on-demand patching |
Fixes #187, adds on-demand patching option to ShadyDOM. Use by setting
ShadyDOM = {noPatch: 'on-demand'}.