-
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 10 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 |
|---|---|---|
|
|
@@ -44,7 +44,18 @@ | |
| }, | ||
| "jshintConfig": { | ||
| "esversion": 6, | ||
| "node": true | ||
| "node": true, | ||
| "globals": { | ||
| "describe": false, | ||
| "xdescribe": false, | ||
| "it": false, | ||
| "xit": false, | ||
| "before": false, | ||
| "beforeEach": false, | ||
| "after": false, | ||
| "afterEach": false, | ||
| "expect": false | ||
| } | ||
|
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. 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Ah, that'll do it. 👍 |
||
| }, | ||
| "engines": { | ||
| "node": ">= 4.0.0" | ||
|
|
||
| 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,42 +16,24 @@ 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 = {}; | ||
| const _upgradedProp = '__$CE_upgraded'; | ||
|
|
||
| /** | ||
| * 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. | ||
| */ | ||
| exports.registerElement = function registerElement(name, options) { | ||
| var nameValidationResult = validateElementName(name); | ||
| if (!nameValidationResult.isValid) { | ||
| throw new Error(`Registration failed for '${name}'. ${nameValidationResult.message}`); | ||
| } | ||
|
|
||
| if (options && options.prototype) { | ||
| registeredElements[name] = options.prototype; | ||
| } else { | ||
| registeredElements[name] = exports.newElement(); | ||
| } | ||
| function transformTree(document, visitedNodes, currentNode, callback) { | ||
|
|
||
| return registeredElements[name].constructor; | ||
| }; | ||
| var task = visitedNodes.has(currentNode) ? undefined : callback(currentNode); | ||
|
|
||
| function recurseTree(rootNode, callback) { | ||
| for (let node of rootNode.childNodes) { | ||
| callback(node); | ||
| recurseTree(node, callback); | ||
| } | ||
| 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. |
||
|
|
||
| let visitChildren = () => Promise.all( | ||
| map(currentNode.childNodes, (child) => transformTree(document, visitedNodes, child, callback)) | ||
| ); | ||
|
|
||
| return Promise.resolve(task).then(visitChildren); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -89,24 +70,27 @@ function renderNode(rootNode) { | |
| let createdPromises = []; | ||
|
|
||
| var document = getDocument(rootNode); | ||
| var visitedNodes = new Set(); | ||
| var customElements = exports.customElements; | ||
|
|
||
| return transformTree(document, visitedNodes, rootNode, function render (element) { | ||
|
|
||
| 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)); | ||
| })); | ||
| } | ||
| const definition = customElements.getDefinition(element.localName); | ||
|
|
||
| if (definition) { | ||
| if ( element[_upgradedProp] ) { | ||
| return; | ||
| } | ||
| } | ||
| }); | ||
| upgradeElement(element, definition, true); | ||
|
|
||
| return Promise.all(createdPromises).then(() => rootNode); | ||
| if (definition.connectedCallback) { | ||
| return new Promise(function(resolve, reject) { | ||
| resolve( definition.connectedCallback.call(element, document) ); | ||
| }); | ||
| } | ||
| } | ||
| }) | ||
| .then(() => rootNode); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -154,3 +138,39 @@ 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. |
||
| var results = []; | ||
| for (var i=0; i < arrayLike.length; i++) { | ||
| results.push( fn(arrayLike[i]) ); | ||
| } | ||
| return results; | ||
| } | ||
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.
Server Components is more or less an implementation of custom elements. I think there's a pretty good argument for not separately namespacing the custom elements API with this as here, and just exposing it at the top-level. I.e. components.define(componentName, Constructor).
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.
True, but the library also provides
renderPageand the like, which isn't conceptually a part ofcustomElementsThere 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.
True. It keeps it simpler and cleaner to have a single entry point though rather than nesting, so I'd prefer the API to be on the one object.
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.
Although it's simpler, I argue it's not cleaner. The
customElementsobject would include extra, unrelated methods, the most notable of which beingHTMLElement. This will matter when isometric elements come into the picture.I pushed the update to a different branch; see the readme to view the difference.
Another benefit is being able to destructure imports. Example:
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.
Sorry, I'd still rather put them on the one object. customElements is just three methods (define, get, whenDefined), almost all users will use
define, and namespacing that separately from the rest of this API is annoying. A lot of users will really only be usingdefineandrenderPage, and they should be up front. Everybody else seems to do the same thing - SkateJS is the closest similar library, and usesskate.define()and even Polymer has moved their [define-equivalent]((https://www.polymer-project.org/1.0/docs/devguide/registering-elements) to the top-level of the API. It's how people expect APIs to work - the key methods should be easily accessible and obvious.On that branch, I don't think you've mapped your changes to exactly how my isometric version works, which might be part of the difference here. Your changes there have renamed the import, seemingly to try and emulate window.customElements. I don't think we want to do that (i.e. you can keep the import as
componentseverywhere). Any component that works with this library has to be aware of it first (by not using the global document object or DOM methods). It's fine to make that explicit and clear, and if we don't then we're shadowing the realcustomElements, which potentially very bad behaviour (what if they have other non-server-component compatible web components on their page, and we break them?).Given that, I don't think it affects the isometric version substantially (we have to map our API to the real methods, and it's easy to do this for the client-side:
components = { define: window.customElements.define, HTMLElement: window.HTMLElement }+ a v1 polyfill), and I'm fine with the resulting destructuring approach too - this way people don't need to understand the custom elements namespacing and they can just import define directlyimport { define, HTMLElement } from 'server-components'.I get that it doesn't match up exactly to reality, but I do think the end result is much nicer as a library interface. Does that difference in how we're looking at isometric elements help explain this?