Convert to custom elements spec v1 #30

gilbert · 2016-11-10T17:44:09Z

Closes #27

The implementation is almost a copy-paste of polymer's polyfill. The main differences is that I removed some unnecessary reactions and hooked in Domino.

If approved, a squash merge would probably be most appropriate.

pimterry

Sorry for the delay - I've now got lots of time all of this week, so I should be able to get back to you with more notes much much quicker.

I've written up a whole bunch of comments. My largest concern is the extends-domino bits, which suggest strange things are going on somewhere, and separating the polyfill from our wrapper code.

Wrapping this all together and getting it out is still going to be a bunch of work, and I'm assuming you'd rather cover these tweaks yourself, but let me know if you'd like me to jump in and start making patches on this too.

All that said, this is a huge chunk of work though, the thrust is great, and I'm really glad you're pushing this forwards. Thanks so much for taking this on!

pimterry · 2016-11-14T12:50:11Z

README.md


 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)`


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).

True, but the library also provides renderPage and the like, which isn't conceptually a part of customElements

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.

Although it's simpler, I argue it's not cleaner. The customElements object would include extra, unrelated methods, the most notable of which being HTMLElement. 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:

var { customElements, HTMLElement } = require('server-components'); // OR import { customElements, HTMLElement } from 'server-components';

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 using define and renderPage, and they should be up front. Everybody else seems to do the same thing - SkateJS is the closest similar library, and uses skate.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 components everywhere). 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 real customElements, 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 directly import { 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?

pimterry · 2016-11-14T12:52:11Z

README.md


-Called when an element is created.
+Called when an element is attached to the faux DOM.


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.

pimterry · 2016-11-14T12:53:55Z

README.md

@@ -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)`


This should be connectedCallback, right?

pimterry · 2016-11-14T12:58:32Z

README.md

@@ -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


This comment is sort-of still true, but doesn't really line up with what's happening here. "Define the class..." is much clearer.

pimterry · 2016-11-14T13:32:00Z

package.json

+      "after": false,
+      "afterEach": false,
+      "expect": false
+    }


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.

Adding "use strict" to the top of the test files caused the linter to start complaining.

Ah, that'll do it. 👍

pimterry · 2016-11-14T14:36:19Z

src/index.js

+//
+// Helpers
+//
+function map (arrayLike, fn) {


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. function asArray(arrayLike) { return [].slice.apply(arrayLike) }

It's neater, but also creates an two extra arrays (an empty one and a copy for the actual mapping).

Can do Array.prototype.slice instead.

Alright, updated.

pimterry · 2016-11-14T14:38:17Z

src/index.js

-};
+    var task = visitedNodes.has(currentNode) ? undefined : callback(currentNode);
+
+    visitedNodes.add(currentNode);


Why is visitedNodes here? It's only useful if we've got loops in the tree, but this is a DOM, so afaik there's no way we can have loops. Is there a case where this is necessary?

It was in the original polyfill. I believe it's possible if a custom element decides to move itself around within the DOM.

Ah, ok. Yes, that makes perfect sense.

pimterry · 2016-11-14T14:49:17Z

src/index.js

+        const definition = customElements.getDefinition(element.localName);
+
+        if (definition) {
+            if ( upgradedNodes.has(element[_upgradedProp]) ) {


I think (from looking at upgradeElement) element[_upgradedProp] is always either undefined or true. That means this test always fails, because the only thing we ever put in upgradedNodes is elements. Am I missing something?

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?

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.

pimterry · 2016-11-14T15:04:06Z

src/index.js

+        CustomElementRegistry.instance()._setNewInstance(element);
+        new (definition.constructor)();
+        element[_upgradedProp] = true;
+        console.assert(CustomElementRegistry.instance()._newInstance === null);


Why do we have this assert in-line? Should probably either be removed or become a test.

It was there from the polyfill. We can probably remove it.

pimterry · 2016-11-14T15:08:00Z

src/registry.js

@@ -0,0 +1,296 @@
+"use strict";


How much of this is the original real polyfill, and how much is extensions on top of it?

If at all possible (and I do get that it might not be) I'd like to keep them separate. It's going to be way easier to maintain this if there's a polyfill file that we can trust (and upgrade) independently, and then a separate bunch of code making the any tweaks required to hook it into Domino, and/or wrapping the polyfill to change the interface.

getDefinition and undefine are extensions. The rest of the changes were to make it compatible with domino instead of the actual DOM.

It'd be useful to keep all those changes separate from the core polyfill though, rather than mixing them all in together. If we can. If you've got examples where we can't possibly make the changes separately then that's ok too, but we should document those, so we can work out what's going on here in future.

But this sort of polyfill modifies the DOM environment. Normally it would modify window and window.document, but since there is no such globals, it modifies domino instead. In other words, modifying domino is a primary concern of the polyfill.

It's fine that it modifies the DOM, my concern is that we're modifying the polyfill from within it, rather than externally, which makes this codebase harder to manage.

What I'm aiming to separate are the modifications to the DOM that any custom elements polyfill would do for any DOM, from both the modifications to Domino we make to get the polyfill working and the modifications to the polyfill we make to provide this library's API.

Some practical reasons:

I'd like to be able to replace the polyfill file in future, to upgrade it without having to worry about recreating changes we've made within it.

At some point hopefully Domino will get built-in custom elements support - I'll need to be able to remove this polyfill completely, and easily see what the extra parts we've built on top are (i.e. getDefinition) so they can be ported to build on top of Domino's implementation instead.

It makes it much easier to review and manage. I shouldn't try and review the polyfill code here now - I should review extensions we're making on top. In future this remains true: if there's a bug in the polyfill, it should be fixed upstream, but if there's a bug in our polyfill wrapper, it should be fixed it here.

It's easier to do all that if we can keep the two as separate as possible.

The changes to the original polyfill were things like this:

const origHTMLElement = win.HTMLElement; // TO const origHTMLElement = domino.impl.HTMLElement;

which were contained in the same file. Are you suggesting we move this to a separate file? Sorry if I'm still misunderstanding.

Focusing on the use cases is the key part of this I think. Given the change you have there, if I wanted to pull in a new version of the polyfill, I'd have to recreate that change, and right now I'd have to do that totally by hand from scratch. That'll be painful, and I'd like to avoid this.

At the very least, some documentation of exactly what we've changed (even perhaps just as git history) would be a start, but separating our changes from the polyfill source entirely is probably doable, I hope. For example, there's other things we can do to wrap a library and inject our own window into it, so you don't need to change the lines as you have there. See https://github.com/pimterry/leaflet-map-server-component/blob/master/src/leaflet-for-server.js, which injects window, document and navigator globals into leaflet, to wrap it without changing the core leaflet code.

This might be a bit fiddly. Have a go, but if you really don't want to dig into this, feel free to commit an unchanged version of the polyfill followed by your changed version, and I'll extract the diff out as much as possible. That also conveniently lets me review the changes we're making to the polyfill, which is an important part of this too.

pimterry · 2016-11-14T18:43:55Z

src/index.js

@@ -1,7 +1,6 @@
 "use strict";

 var domino = require("domino");
-var validateElementName = require("validate-element-name");


Just spotted this - we should remove the dependency if we're not using this any more.

pimterry · 2016-11-14T18:51:56Z

test/programmatic-usage-test.js

@@ -6,9 +6,11 @@ var components = require("../src/index.js");
 describe("Programmatic usage", () => {

    // Pending until we decide what we want from this
-    xit("returns the element constructor from the registration call", () => {
+    it("returns the element constructor from the registration call", () => {


Test changes look great, but the name needs updating too.

pimterry · 2016-11-14T18:53:21Z

src/index.js

-};
+    var task = visitedNodes.has(currentNode) ? undefined : callback(currentNode);
+
+    visitedNodes.add(currentNode);


Ah, ok. Yes, that makes perfect sense.

pimterry · 2016-11-15T14:25:54Z

Just an update to keep track of where we are. Outstanding issues are:

Moving methods out of the .customElements namespace
Disabled child prototype attributes test needs fixing
Separating out the original polyfill from our changes to it.
The broken extend-domino code. This is waiting on Domino, and in need of some tests here in the meantime (which will probably failing right now, but that should have passing browser equivalents) to confirm the behaviour this code provides (I think that means testing adding custom elements to nodes after traversal).

I think all the other comments are covered or dealt with (but let me know if I've missed something).

gilbert · 2016-11-15T17:16:46Z

1 and 2 are complete. For 3, I now understand what you mean by separating out the polyfill. However, server-components expands on the spec by looking at the return value of connectedCallback within renderNode. It seems in order to have a clear separation, the component will have to register promises some other way, e.g. customElements.wait(new Promise...) instead of return new Promise...

pimterry · 2016-11-16T12:34:51Z

On part 1, in your latest commit we've still got the changes I mentioned here. We shouldn't be renaming the import to customElements everywhere. It should just be:

var components = import("server-components");
class X extends components.HTMLElement { ... }
components.define(...)

We're building a new API that's extremely easy to migrate to from custom elements, but we're definitely not emulating custom elements exactly (because that's not possible), and we shouldn't give that impression.

pimterry · 2016-11-16T16:29:28Z

On your part 3 comment:

server-components expands on the spec by looking at the return value of connectedCallback within renderNode.

There's ways we can do this from outside the polyfill. For example, we can easily externally override just _addElement on the registry to define our behaviour for that part instead:

polyfill-extensions.js

CustomElementRegistry.prototype._addElement = [do same thing, but think about promises too]

Alternatively, we could add behavior inside our define() API, and transform the component's callbacks before they're added to the registry, so that anytime that the polyfill calls a callback, it runs our code after the real callback code and checks for a promise returned, and then tracks that elsewhere.

That way looks nicer actually: we use exactly the same polyfill logic during upgrade and element creation, but we automatically insert hooks into the callback interface to track their status when defining them. Conceptually tidy.

Either way, changes like this makes it clear that we haven't changed the rest of the polyfill, makes it fairly obvious which changes we have made on top (i.e. everything is the same, but we use different behaviour at add-element time, or in component callbacks), and makes it easier to review this and change it in future.

I think this probably works best with one component registry per render, which might actually be a good idea generally. I'm a bit cautious of sharing registries between renders, since there's lots of potential for weird interactions, and it's not really what any of this polyfill code was designed to do (whereas rendering a single document is exactly what it was designed for).

Again, this is tricky! Let me know if you rather I dived in myself.

gilbert · 2016-11-16T21:20:50Z

Thank you for the explanation, I think I now understand what you mean.

The original polyfill also had MutationObservers. Do you want to strip those out, provide a mock implementation, or wait for / make a PR for domino to implement them?

pimterry · 2016-11-17T10:45:47Z

Good point. I've actually got an open issue with domino looking at adding mutation observers, and they're interested, if we provide the implementation.

This will be a bigger chunk of work, and I'll get on it this week, but it's certainly not going to be done today. If you assume for your changes here that MutationObserver works though, I'll try and get an implementation put together at least by the start of next week. Hope that doesn't block you too much in the meantime!

pimterry · 2016-11-29T16:31:42Z

A quick update on the status of this again. I think we're down to:

Fix the extend-domino code.

This is still waiting on my Domino PR, but also needs tests for the functionality it's trying to provide.

I think that means testing what happens if you instantiate an element with Document.createElement("my-element"), to make sure it returns an actual instance of that element. I think that's what _createElement in registry.js is doing, it's just that without extend-domino working we can't properly replace createElement with it. If you could add the test though, then we can check it fails correctly now, and it should start working as soon as Domino merge that PR.
Updating to properly use the real polyfill code.

I've been working on MutationObservers, but it turns out there's a huge chunk of work here just getting the official mutation observer tests running against Domino, which I'm slugging through at the moment, and getting the whole thing together is going to take quite some time. On top of that, Domino don't seem to be merging things super quickly.

I propose we shelve this for the moment. I really want to move this onto a proper polyfill in a manageable way, but I don't want to block v1 support for that, and this is already better than it was anyway. We can track this on Move to wrapping a standard custom elements polyfill #21 for the future.

mattjburrows · 2017-02-25T12:57:09Z

Hi I'm trying to follow the thread of where this PR is at.

I take it the "Fix the extend-domino code" task is complete as per this merge? Does that just leave the "updating to properly use the real polyfill code" task?

Is there anything I can help out with?

gilbert · 2017-02-25T20:33:42Z

I'm not maintaining this PR any more; I've found what I was looking for in Marko.js's upcoming v4 release. Feel free to take over.

gilbert added 5 commits November 7, 2016 17:33

Implement server-side components

af9591e

Convert to custom elements spec v1. Closes pimterry#27

2671b99

Add docs

63ad32a

Remove server-side components feature for now

88aaf5d

Make travis happy

329eff3

gilbert force-pushed the elems-v1 branch from a751740 to 329eff3 Compare November 10, 2016 19:25

Refactor to support older node versions

568ee36

gilbert force-pushed the elems-v1 branch from b6f6ca8 to 568ee36 Compare November 10, 2016 19:42

pimterry suggested changes Nov 14, 2016

View reviewed changes

gilbert added 4 commits November 14, 2016 11:19

Updates per @pimterry's review

106d66a

Testing: Reset all instead of individual definitions

0b5a395

Remove excessive tracking

722040b

Remove assert

723c656

pimterry reviewed Nov 14, 2016

View reviewed changes

gilbert added 2 commits November 14, 2016 13:58

Change map

959bff7

Remove dependency

71829e6

gilbert added 2 commits November 15, 2016 10:43

Set define as the top level

3f7c02e

Update & pass child-accessing test

374b24f

gilbert force-pushed the elems-v1 branch from af7c7c0 to 374b24f Compare November 15, 2016 17:08

gilbert force-pushed the elems-v1 branch from 46946ea to b82cec5 Compare November 16, 2016 21:17

customElements -> components

1a9c7d3

gilbert force-pushed the elems-v1 branch from b82cec5 to 1a9c7d3 Compare November 16, 2016 21:20

pimterry mentioned this pull request Nov 24, 2016

Revisit server-side rendering skatejs/skatejs#573

Closed


		Called when an element is created.
		Called when an element is attached to the faux DOM.

		@@ -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

Convert to custom elements spec v1 #30

Are you sure you want to change the base?

Convert to custom elements spec v1 #30

Conversation

gilbert commented Nov 10, 2016 • edited Loading

pimterry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pimterry commented Nov 15, 2016 • edited Loading

gilbert commented Nov 15, 2016

pimterry commented Nov 16, 2016

pimterry commented Nov 16, 2016

gilbert commented Nov 16, 2016 • edited Loading

pimterry commented Nov 17, 2016

pimterry commented Nov 29, 2016

mattjburrows commented Feb 25, 2017

gilbert commented Feb 25, 2017

gilbert commented Nov 10, 2016 •

edited

Loading

pimterry commented Nov 15, 2016 •

edited

Loading

gilbert commented Nov 16, 2016 •

edited

Loading