server-components icon indicating copy to clipboard operation
server-components copied to clipboard

Convert to custom elements spec v1

Open gilbert opened this issue 7 years ago • 9 comments

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.

gilbert avatar Nov 10 '16 17:11 gilbert

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

  1. [x] Moving methods out of the .customElements namespace
  2. [x] Disabled child prototype attributes test needs fixing
  3. [ ] Separating out the original polyfill from our changes to it.
  4. [ ] 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).

pimterry avatar Nov 15 '16 14:11 pimterry

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

gilbert avatar Nov 15 '16 17:11 gilbert

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 avatar Nov 16 '16 12:11 pimterry

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.

pimterry avatar Nov 16 '16 16:11 pimterry

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?

gilbert avatar Nov 16 '16 21:11 gilbert

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 avatar Nov 17 '16 10:11 pimterry

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 #21 for the future.

pimterry avatar Nov 29 '16 16:11 pimterry

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?

mattjburrows avatar Feb 25 '17 12:02 mattjburrows

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 avatar Feb 25 '17 20:02 gilbert