-
Notifications
You must be signed in to change notification settings - Fork 12
Convert to custom elements spec v1 #30
base: master
Are you sure you want to change the base?
Changes from all commits
af9591e
2671b99
63ad32a
88aaf5d
329eff3
568ee36
106d66a
0b5a395
722040b
723c656
959bff7
71829e6
3f7c02e
374b24f
1a9c7d3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
// | ||
// Strict mode disallows us to overwrite Document.prototype properties. | ||
// This file is to stay out of strict mode. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These strict mode errors are happening because these properties have been defined with writable set to false somewhere. Strict mode doesn't aim to change working runtime behaviour - it just exposes issues that are otherwise hidden. Those errors are appearing here because these writes don't actually do anything - they're silently failing. You're not successfully changing createElement here. I'm not totally clear on the goal of this code, but I've had a quick test, and if you remove 'createElement' and 'createElementNS' below here then you can enable strict mode on this file, and all the tests still pass. That suggests either there's a bunch more code involved here (like _createElement) that we could delete too, or that we're missing tests that cover whatever this is doing. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see. My understanding is that the code is supposed to support |
||
// | ||
var domino = require("domino"); | ||
var Document = require('domino/lib/Document'); | ||
var Element = require('domino/lib/Element'); | ||
|
||
|
||
module.exports = function (newHTMLElement, _createElement) { | ||
var result = {}; | ||
|
||
// | ||
// Patch document.createElement | ||
// | ||
Document.prototype.createElement = function(tagName, options) { | ||
return _createElement(this, tagName, options, true); | ||
}; | ||
|
||
// | ||
// Patch HTMLElement | ||
// | ||
result.HTMLElement = newHTMLElement; | ||
result.HTMLElement.prototype = Object.create(domino.impl.HTMLElement.prototype, { | ||
constructor: {value: result.HTMLElement, configurable: true, writable: true}, | ||
}); | ||
|
||
|
||
// | ||
// Patch doc.createElementNS | ||
// | ||
var HTMLNS = 'http://www.w3.org/1999/xhtml'; | ||
var _origCreateElementNS = Document.prototype.createElementNS; | ||
|
||
Document.prototype.createElementNS = function(namespaceURI, qualifiedName) { | ||
if (namespaceURI === 'http://www.w3.org/1999/xhtml') { | ||
return this.createElement(qualifiedName); | ||
} else { | ||
return _origCreateElementNS.call(this, namespaceURI, qualifiedName); | ||
} | ||
}; | ||
|
||
return result; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we're keeping this, it needs some comments. What are these patches doing to Domino's built-in behaviour? Why doesn't Domino's DOM + the polyfill do what we want already? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was just following what the original polyfill did. I believe this is supposed to support programatically creating custom elements. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interesting, ok. We should add tests for that then. If this code is necessary, that's probably broken, because this code doesn't work. The right answer to this might well be that these properties are writable in a browser, but not in Domino. That's probably not supposed to be that case, so we should talk to Domino, make this writable there, and then everything'll be fine. Can you check that that's the problem? If so, I'm happy to look at sorting this in Domino. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with you, that is the right approach to take. The browser does in fact allow you to override |
||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,6 @@ | ||
"use strict"; | ||
|
||
var domino = require("domino"); | ||
var validateElementName = require("validate-element-name"); | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just spotted this - we should remove the dependency if we're not using this any more. |
||
/** | ||
* The DOM object (components.dom) exposes tradition DOM objects (normally globally available | ||
|
@@ -17,41 +16,39 @@ exports.dom = domino.impl; | |
* with an element name, and options (typically including the prototype returned here as your | ||
* 'prototype' value). | ||
*/ | ||
exports.newElement = function newElement() { | ||
return Object.create(domino.impl.HTMLElement.prototype); | ||
}; | ||
var CustomElementRegistry = require('./registry'); | ||
exports.customElements = CustomElementRegistry.instance(); | ||
exports.HTMLElement = CustomElementRegistry.HTMLElement; | ||
|
||
var registeredElements = {}; | ||
|
||
/** | ||
* Registers an element, so that it will be used when the given element name is found during parsing. | ||
* | ||
* Element names are required to contain a hyphen (to disambiguate them from existing element names), | ||
* be entirely lower-case, and not start with a hyphen. | ||
* | ||
* The only option currently supported is 'prototype', which sets the prototype of the given element. | ||
* This prototype will have its various callbacks called when it is found during document parsing, | ||
* and properties of the prototype will be exposed within the DOM to other elements there in turn. | ||
* Re-export methods for convenience | ||
*/ | ||
exports.registerElement = function registerElement(name, options) { | ||
var nameValidationResult = validateElementName(name); | ||
if (!nameValidationResult.isValid) { | ||
throw new Error(`Registration failed for '${name}'. ${nameValidationResult.message}`); | ||
} | ||
exports.define = function (name, constructor, options) { | ||
return CustomElementRegistry.instance().define(name, constructor, options); | ||
}; | ||
exports.get = function (name) { | ||
return CustomElementRegistry.instance().get(name); | ||
}; | ||
exports.whenDefined = function (name) { | ||
return CustomElementRegistry.instance().whenDefined(name); | ||
}; | ||
exports.reset = function (name) { | ||
return CustomElementRegistry.instance().reset(); | ||
}; | ||
|
||
if (options && options.prototype) { | ||
registeredElements[name] = options.prototype; | ||
} else { | ||
registeredElements[name] = exports.newElement(); | ||
} | ||
|
||
return registeredElements[name].constructor; | ||
}; | ||
const _upgradedProp = '__$CE_upgraded'; | ||
|
||
|
||
function transformTree(document, visitedNodes, currentNode, callback) { | ||
|
||
var task = visitedNodes.has(currentNode) ? undefined : callback(currentNode); | ||
|
||
visitedNodes.add(currentNode); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It was in the original polyfill. I believe it's possible if a custom element decides to move itself around within the DOM. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, ok. Yes, that makes perfect sense. |
||
|
||
function recurseTree(rootNode, callback) { | ||
for (let node of rootNode.childNodes) { | ||
callback(node); | ||
recurseTree(node, callback); | ||
for (var child of currentNode.childNodes) { | ||
transformTree(document, visitedNodes, child, callback); | ||
} | ||
} | ||
|
||
|
@@ -89,24 +86,28 @@ function renderNode(rootNode) { | |
let createdPromises = []; | ||
|
||
var document = getDocument(rootNode); | ||
var visitedNodes = new Set(); | ||
var customElements = exports.customElements; | ||
|
||
recurseTree(rootNode, (foundNode) => { | ||
if (foundNode.tagName) { | ||
let nodeType = foundNode.tagName.toLowerCase(); | ||
let customElement = registeredElements[nodeType]; | ||
if (customElement) { | ||
// TODO: Should probably clone node, not change prototype, for performance | ||
Object.setPrototypeOf(foundNode, customElement); | ||
if (customElement.createdCallback) { | ||
createdPromises.push(new Promise((resolve) => { | ||
resolve(customElement.createdCallback.call(foundNode, document)); | ||
})); | ||
} | ||
transformTree(document, visitedNodes, rootNode, function render (element) { | ||
|
||
const definition = customElements.getDefinition(element.localName); | ||
|
||
if (definition) { | ||
if ( element[_upgradedProp] ) { | ||
return; | ||
} | ||
upgradeElement(element, definition, true); | ||
|
||
if (definition.connectedCallback) { | ||
var p = new Promise(function(resolve, reject) { | ||
resolve( definition.connectedCallback.call(element, document) ); | ||
}); | ||
createdPromises.push(p); | ||
} | ||
} | ||
}); | ||
|
||
return Promise.all(createdPromises).then(() => rootNode); | ||
return Promise.all(createdPromises).then(function(){ return rootNode; }); | ||
} | ||
|
||
/** | ||
|
@@ -154,3 +155,35 @@ function getDocument(rootNode) { | |
return rootNode; | ||
} | ||
} | ||
|
||
function upgradeElement (element, definition, callConstructor) { | ||
const prototype = definition.constructor.prototype; | ||
Object.setPrototypeOf(element, prototype); | ||
if (callConstructor) { | ||
CustomElementRegistry.instance()._setNewInstance(element); | ||
new (definition.constructor)(); | ||
element[_upgradedProp] = true; | ||
} | ||
|
||
const observedAttributes = definition.observedAttributes; | ||
const attributeChangedCallback = definition.attributeChangedCallback; | ||
if (attributeChangedCallback && observedAttributes.length > 0) { | ||
|
||
// Trigger attributeChangedCallback for existing attributes. | ||
// https://html.spec.whatwg.org/multipage/scripting.html#upgrades | ||
for (let i = 0; i < observedAttributes.length; i++) { | ||
const name = observedAttributes[i]; | ||
if (element.hasAttribute(name)) { | ||
const value = element.getAttribute(name); | ||
attributeChangedCallback.call(element, name, null, value, null); | ||
} | ||
} | ||
} | ||
} | ||
|
||
// | ||
// Helpers | ||
// | ||
function map (arrayLike, fn) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's neater to just convert the array-like into a real array, and then use real map, rather than reimplementing map and any other functions we need all from scratch. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's neater, but also creates an two extra arrays (an empty one and a copy for the actual mapping). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can do Array.prototype.slice instead. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Alright, updated. |
||
return Array.prototype.slice.call(arrayLike).map(fn); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. This makes perfect sense, and I'm not sure why been it's passing without it all this time! Any idea? Right now, it seems to pass fine on my machine and in CI without this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding "use strict" to the top of the test files caused the linter to start complaining.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, that'll do it. 👍