-
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 6 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 |
---|---|---|
|
@@ -27,19 +27,19 @@ You can take the same ideas (and standards), apply them directly server side, to | |
var components = require("server-components"); | ||
|
||
// Get the prototype for a new element | ||
var NewElement = components.newElement(); | ||
|
||
// When the element is created during DOM parsing, you can transform the HTML inside it. | ||
// This can be configurable too, either by setting attributes or adding HTML content | ||
// inside it or elsewhere in the page it can interact with. Elements can fire events | ||
// that other elements can receive to allow interactions, or even expose methods | ||
// or data that other elements in the page can access directly. | ||
NewElement.createdCallback = function () { | ||
this.innerHTML = "Hi there"; | ||
}; | ||
class NewElement extends components.HTMLElement { | ||
// When the element is created during DOM parsing, you can transform the HTML inside it. | ||
// This can be configurable too, either by setting attributes or adding HTML content | ||
// inside it or elsewhere in the page it can interact with. Elements can fire events | ||
// that other elements can receive to allow interactions, or even expose methods | ||
// or data that other elements in the page can access directly. | ||
connectedCallback() { | ||
this.innerHTML = "Hi there"; | ||
} | ||
} | ||
|
||
// Register the element with an element name | ||
components.registerElement("my-new-element", { prototype: NewElement }); | ||
components.customElements.define("my-new-element", NewElement); | ||
``` | ||
|
||
For examples of more complex component definitions, take a look at the [example components](https://github.com/pimterry/server-components/blob/master/component-examples.md) | ||
|
@@ -83,15 +83,15 @@ There aren't many published sharable components to drop in quite yet, as it's st | |
|
||
### Top-level API | ||
|
||
#### `components.newElement()` | ||
#### `components.HTMLElement` | ||
|
||
Creates a returns a new custom HTML element prototype, extending the HTMLElement prototype. | ||
|
||
Note that this does *not* register the element. To do that, call `components.registerElement` with an element name, and options (typically including the prototype returned here as your 'prototype' value). | ||
|
||
This is broadly equivalent to `Object.create(HTMLElement.prototype)` in browser land, and exactly equivalent here to `Object.create(components.dom.HTMLElement.prototype)`. You can call that yourself instead if you like, but it's a bit of a mouthful. | ||
|
||
#### `components.registerElement(componentName, options)` | ||
#### `components.customElements.define(componentName, Constructor)` | ||
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. 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 commentThe reason will be displayed to describe this comment to others. Learn more. True, but the library also provides 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. 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 commentThe reason will be displayed to describe this comment to others. Learn more. Although it's simpler, I argue it's not cleaner. The I pushed the update to a different branch; see the readme to view the difference. Another benefit is being able to destructure imports. Example: var { customElements, HTMLElement } = require('server-components');
// OR
import { customElements, HTMLElement } from 'server-components'; 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. Sorry, I'd still rather put them on the one object. customElements is just three methods (define, get, whenDefined), almost all users will use 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 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: 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? |
||
|
||
Registers an element, so that it will be used when the given element name is found during parsing. | ||
|
||
|
@@ -131,9 +131,9 @@ These methods are methods you can implement on your component prototype (as retu | |
|
||
Any methods that are implemented, from this selection or otherwise, will be exposed on your element in the DOM during rendering. I.e. you can call `document.querySelector("my-element").setTitle("New Title")` and to call the `setTitle` method on your object, which can then potentially change how your component is rendered. | ||
|
||
#### `yourComponent.createdCallback(document)` | ||
#### `yourComponentConstructor.prototype.createdCallback(document)` | ||
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. This should be |
||
|
||
Called when an element is created. | ||
Called when an element is attached to the faux 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. It's not a faux DOM, it's a real DOM. Domino's a real DOM implementation, just one that doesn't happen to be tied to a layout engine. More practically, this API should hopefully be the same in server-components-for-web, where this same callback will be called after attachment to a completely normal browser DOM, and it'd be nice to be consistent. |
||
|
||
**This is where you put your magic!** Rewrite the elements contents to dynamically generate what your users will actually see client side. Read configuration from attributes or the initial child nodes to create flexible reconfigurable reusable elements. Register for events to create elements that interact with the rest of the application structure. Build your page. | ||
|
||
|
@@ -143,17 +143,13 @@ If this callback returns a promise, the rendering process will not resolve until | |
|
||
These callbacks are called in opening tag order, so a parent's createdCallback is called, then each of its children's, then its next sibling element. | ||
|
||
#### `yourComponent.attachedCallback(document)` | ||
|
||
Called when the element is attached to the DOM. This is different to when it's created when your component is being built programmatically, not through HTML parsing. *Not yet implemented* | ||
|
||
#### `yourComponent.detachedCallback(document)` | ||
#### `yourComponentConstructor.prototype.disconnectedCallback(document)` | ||
|
||
Called when the element is removed from the DOM. *Not yet implemented* | ||
|
||
#### `yourComponent.attributeChangedCallback(document)` | ||
#### `yourComponentConstructor.prototype.attributeChangedCallback(document)` | ||
|
||
Called when an attribute of the element is added, changed, or removed. *Not yet implemented*. | ||
Called when an attribute of the element is added, changed, or removed. *Partially implemented;* runs on component initialization. | ||
|
||
**So far only the createdCallback is implemented here, as the others are less relevant initially for the key simpler cases. Each of those will be coming in time though! Watch this space.** | ||
|
||
|
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,41 +16,28 @@ 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); | ||
|
||
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)) | ||
); | ||
|
||
function recurseTree(rootNode, callback) { | ||
for (let node of rootNode.childNodes) { | ||
callback(node); | ||
recurseTree(node, callback); | ||
if ( task && task.then ) { | ||
return task.then(visitChildren); | ||
} | ||
else { | ||
return visitChildren(); | ||
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. This last if/else chunk can just be |
||
} | ||
} | ||
|
||
|
@@ -89,24 +75,29 @@ function renderNode(rootNode) { | |
let createdPromises = []; | ||
|
||
var document = getDocument(rootNode); | ||
var visitedNodes = new Set(); | ||
var upgradedNodes = 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 ( upgradedNodes.has(element[_upgradedProp]) ) { | ||
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 think (from looking at upgradeElement) Feels like we're mixing two upgrade trackers here: a list of elements and a property on the elements. In fact, can't we drop both of them, and just consider an element upgraded if it already has the correct prototype set? 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 must have mistranslated this from the polyfill. You're right in that there is excessive tracking going on here. My guess on why they opted for setting a property is that comparing prototypes might be more expensive. But that's just a guess. |
||
return; | ||
} | ||
} | ||
}); | ||
upgradeElement(element, definition, true); | ||
upgradedNodes.add(element); | ||
|
||
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 +145,40 @@ 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; | ||
console.assert(CustomElementRegistry.instance()._newInstance === null); | ||
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 do we have this assert in-line? Should probably either be removed or become a test. 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 there from the polyfill. We can probably remove it. |
||
} | ||
|
||
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.
This comment is sort-of still true, but doesn't really line up with what's happening here. "Define the class..." is much clearer.