New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add native support for custom elements. #715

Open
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@treshugart

treshugart commented May 31, 2017

This requires Babel 6+ because it fixes extending HTMLElement, which is required for the tests to run. For that reason, I've based these changes off this dependant PR: #683.

Rationale

It's desirable to be able to pass a custom element constructor as the node name of a virtual element, just as you can with functions and components.

While the ability to do this is aesthetic, it enables consumers of both Preact components and custom elements (written with any library) to treat them as the same thing, for all intents and purposes. Furthermore, it means that no matter who registers the custom element, the Preact-based consumer doesn't need to worry about what it was called, it simply uses the exported constructor.

For example, this is what you can do with this change:

import PreactComponent from 'preact-component';
import WebComponent from 'web-component';

export default (
  <div>
    <PreactComponent>
      <WebComponent />
    </PreactComponent>
  </div>
);

As opposed to what you currently have to do:

import PreactComponent from 'preact-component';

// We're assuming it's already defined as "web-component".
import 'web-component';

export default (
  <div>
    <PreactComponent>
      <web-component />
    </PreactComponent>
  </div>
);

Implementation details

This was implemented at the diff level so that it could efficiently, and correctly, check based on constructor equivalence, as opposed to somehow determining the node name and reusing the existing algorithms. This is both more efficient than the aforementioned and spec compliant.

Testing

I wrote these as separate custom-elements browser tests. While you can quite easily patch Undom to work with them, the target is still ultimately the browser.

I thought it was best to also test not only the node-to-node diffing and patching, but also how it behaved with keyed nodes and unkeyed nodes (plucking, as it's called in the source) because there are separate code paths for elements and descending into child lists.

Most tests seem to test integration between source files as opposed to units within them, gaining coverage implicitly. Many favour this approach, so I opted to follow that, convention or not. Happy to add specific unit tests if you feel it's necessary.

I've decided to only test against native versions of custom elements to reduce the amount of polyfills one must include, and amount of variables they could introduce. Since native implementations require the use of ES2015 classes, and the source / tests are transpiled to ES5, this requires the use of the ES5 adapter which I've had to include in this PR because Karma doesn't allow the dynamic inclusion of files from a CDN (as far as I could tell).

Notes

There were several minor refactors along the way to consolidate certain checks into functions, and to reuse a bit more code. I'll annotate the parts that I've done this to along with my rationale.

Performance

I ran the tests and noted the performance both before the changes, and after.

Before:

LOG: 'PERF: empty diff: 18971/s (139 ticks)'
  performance
    ✔ should rerender without changes fast
LOG: 'PERF: repeat diff: 4459/s (735 ticks)'
    ✔ should rerender repeated trees fast
LOG: 'PERF: large VTree: 4554/s (731 ticks)'
    ✔ should construct large VDOM trees fast
LOG: 'PERF: style/prop mutation: 12100/s (268 ticks)'
    ✔ should mutate styles/properties quickly
LOG: 'PERF: SSR Hydrate: 1424/s c

After:

LOG: 'PERF: empty diff: 19062/s (167 ticks)'
  performance
    ✔ should rerender without changes fast
LOG: 'PERF: repeat diff: 4231/s (783 ticks)'
    ✔ should rerender repeated trees fast
LOG: 'PERF: large VTree: 4463/s (745 ticks)'
    ✔ should construct large VDOM trees fast
LOG: 'PERF: style/prop mutation: 13498/s (246 ticks)'
    ✔ should mutate styles/properties quickly
LOG: 'PERF: SSR Hydrate: 1479/s (2247 ticks)'
    ✔ should hydrate from SSR quickly

As you can see, the ones after the changes are actually an improvement. They're so close, this could be due to environmental variables, but nonetheless, it's a good thing!

Related

Fixes #704.

feat: Add native support for custom elements.
You can now pass custom element constructors as the node name of a virtual element. This was implemented at the diff level so that it could efficiently, and correctly, check based on constructors as opposed to somehow determining the node name and reusing the existing algorithms. This is both more efficient than the aforementioned and spec compliant.
@@ -0,0 +1,15 @@
{

This comment has been minimized.

@treshugart
@treshugart
Show outdated Hide outdated package.json
* @param {Boolean} [isSvg=false] If `true`, creates an element within the SVG namespace.
* @returns {Element} node
*/
export function createNode(nodeName, isSvg) {
let node = isSvg ? document.createElementNS('http://www.w3.org/2000/svg', nodeName) : document.createElement(nodeName);
let node;
if (typeof nodeName==='function') {

This comment has been minimized.

@treshugart

treshugart May 31, 2017

This now has a branch so that it can create a custom element via newing the constructor. This covers both custom elements and customised built-in elements (is attribute) as it will correctly create the element based on how it was defined.

@treshugart

treshugart May 31, 2017

This now has a branch so that it can create a custom element via newing the constructor. This covers both custom elements and customised built-in elements (is attribute) as it will correctly create the element based on how it was defined.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls May 31, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 3927012 on treshugart:master into b80bec3 on developit:master.

coveralls commented May 31, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 3927012 on treshugart:master into b80bec3 on developit:master.

Show outdated Hide outdated src/util.js
Show outdated Hide outdated src/util.js
@@ -0,0 +1,18 @@
/* eslint-disable */

This comment has been minimized.

@treshugart

treshugart May 31, 2017

This is the ES5 adapter needed to use ES5 "classes" with native custom elements.

@treshugart

treshugart May 31, 2017

This is the ES5 adapter needed to use ES5 "classes" with native custom elements.

// We can't load this up front because it's ES2015 and we need it only
// for certain tests that run under those conditions. We also can't load
// it via CDN because { included: false } won't work.
{ pattern: 'custom-elements-es5-adapter.js', included: false },

This comment has been minimized.

@treshugart

treshugart May 31, 2017

Unfortunately, I couldn't find a way to get karma to conditionally load stuff from a CDN, so I had to include it.

@treshugart

treshugart May 31, 2017

Unfortunately, I couldn't find a way to get karma to conditionally load stuff from a CDN, so I had to include it.

@@ -3,3 +3,11 @@ import 'core-js/es6/map';
import 'core-js/fn/array/fill';
import 'core-js/fn/array/from';
import 'core-js/fn/object/assign';
// We add the ES5 adapter because src / test are converted to ES5 and native

This comment has been minimized.

@treshugart

treshugart May 31, 2017

Comment is self-explanatory, but thought I should highlight this as part of the caveats listed in the main description of the PR.

@treshugart

treshugart May 31, 2017

Comment is self-explanatory, but thought I should highlight this as part of the caveats listed in the main description of the PR.

This comment has been minimized.

@trusktr

trusktr Feb 2, 2018

@WebReflection's ideas from babel-plugin-transform-builtin-classes have been incorporated into Babel 7, so at some point this adapter will not be needed.

I already use WebComponents.js without native-shim in my projects, transpiling with Babel 6 + babel-plugin-transform-builtin-classes.

@trusktr

trusktr Feb 2, 2018

@WebReflection's ideas from babel-plugin-transform-builtin-classes have been incorporated into Babel 7, so at some point this adapter will not be needed.

I already use WebComponents.js without native-shim in my projects, transpiling with Babel 6 + babel-plugin-transform-builtin-classes.

This comment has been minimized.

@trusktr

trusktr Feb 2, 2018

The result of using babel-plugin-transform-builtin-classes is nice because the transpiled code runs in all browsers (old and new).

@trusktr

trusktr Feb 2, 2018

The result of using babel-plugin-transform-builtin-classes is nice because the transpiled code runs in all browsers (old and new).

Show outdated Hide outdated package.json
Show outdated Hide outdated src/util.js
Show outdated Hide outdated src/vdom/index.js
Show outdated Hide outdated src/vdom/index.js
Show outdated Hide outdated src/vdom/index.js
@developit

This comment has been minimized.

Show comment
Hide comment
@developit

developit Jun 3, 2017

Owner

I need to spend some time with this one. There are things in here that could impact the non-WC use-cases and I'd like to write some tests / benches around those to be very sure.

Definitely a vote for the duck-type checking of HTMLElement - that's super important, we had to do the same thing for Text in 8.x.

Owner

developit commented Jun 3, 2017

I need to spend some time with this one. There are things in here that could impact the non-WC use-cases and I'd like to write some tests / benches around those to be very sure.

Definitely a vote for the duck-type checking of HTMLElement - that's super important, we had to do the same thing for Text in 8.x.

@treshugart

This comment has been minimized.

Show comment
Hide comment
@treshugart

treshugart Jun 3, 2017

@developit let me know what I can help with here. I think it's important this gets in, so anything I can do to help.

treshugart commented Jun 3, 2017

@developit let me know what I can help with here. I think it's important this gets in, so anything I can do to help.

@treshugart

This comment has been minimized.

Show comment
Hide comment
@treshugart

treshugart Jun 3, 2017

Re the duck typing, I'll check. The prototype check is more robust. It's also worth noting that I don't think undom implements either of these, so I wonder if something like 'nodeName' in prototype check is better?

treshugart commented Jun 3, 2017

Re the duck typing, I'll check. The prototype check is more robust. It's also worth noting that I don't think undom implements either of these, so I wonder if something like 'nodeName' in prototype check is better?

@developit

This comment has been minimized.

Show comment
Hide comment
@developit

developit Jun 3, 2017

Owner

ooh yeah 'nodeName' in prototype would be better for sure, or typeof nodeName.prototype.removeAttribute!=='undefined'.

Owner

developit commented Jun 3, 2017

ooh yeah 'nodeName' in prototype would be better for sure, or typeof nodeName.prototype.removeAttribute!=='undefined'.

@developit

This comment has been minimized.

Show comment
Hide comment
@developit

developit Jun 3, 2017

Owner

I'll try to comb through some more, just I may need to play with this quite a bit.

Owner

developit commented Jun 3, 2017

I'll try to comb through some more, just I may need to play with this quite a bit.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jun 6, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 3e9f2dc on treshugart:master into 8611fbc on developit:master.

coveralls commented Jun 6, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 3e9f2dc on treshugart:master into 8611fbc on developit:master.

@treshugart

This comment has been minimized.

Show comment
Hide comment
@treshugart

treshugart Jun 6, 2017

Ok, so I've done the following:

  • Removed destructures and favoured direct property access
  • Removed lowercase() function and just call .toLowerCase() directly
  • Used 'nodeName' in vnodeName.prototype to check if it's a custom element constructor being passed in

If I tried moving the custom element check up where you suggested (where it checks if the vnodeName is a string), the tests failed. I think it's probably okay where it is because you're essentially using the custom element like a component constructor, thus you want the same check to apply, no?

EDIT

I just rebased and squashed the refactors so there's no breaking commits in between.

treshugart commented Jun 6, 2017

Ok, so I've done the following:

  • Removed destructures and favoured direct property access
  • Removed lowercase() function and just call .toLowerCase() directly
  • Used 'nodeName' in vnodeName.prototype to check if it's a custom element constructor being passed in

If I tried moving the custom element check up where you suggested (where it checks if the vnodeName is a string), the tests failed. I think it's probably okay where it is because you're essentially using the custom element like a component constructor, thus you want the same check to apply, no?

EDIT

I just rebased and squashed the refactors so there's no breaking commits in between.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jun 6, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 6b9a2c7 on treshugart:master into 8611fbc on developit:master.

coveralls commented Jun 6, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 6b9a2c7 on treshugart:master into 8611fbc on developit:master.

refactor: refactor PR to implement review comments.
- No destructuring
- No lowercase() function, use .toLowerCase() instead
- Favour property access (no variable saving, unless necessary)
- Use `'nodeName' in vnodeName.prototype` instead of HTMLElement instance checking
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jun 6, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling c90d907 on treshugart:master into 8611fbc on developit:master.

coveralls commented Jun 6, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling c90d907 on treshugart:master into 8611fbc on developit:master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jun 6, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling c90d907 on treshugart:master into 8611fbc on developit:master.

coveralls commented Jun 6, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling c90d907 on treshugart:master into 8611fbc on developit:master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 23, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 3d68b2a on treshugart:master into 0dea3b7 on developit:master.

coveralls commented Aug 23, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 3d68b2a on treshugart:master into 0dea3b7 on developit:master.

}
return hydrating || node._componentConstructor===vnode.nodeName;
return hydrating || node._componentConstructor===vnode.nodeName || node.constructor === vnode.nodeName;

This comment has been minimized.

@developit

developit May 14, 2018

Owner

Q: @treshugart - any idea if this will fail prior to CE registration?

Sorry for the delays on this PR.

@developit

developit May 14, 2018

Owner

Q: @treshugart - any idea if this will fail prior to CE registration?

Sorry for the delays on this PR.

This comment has been minimized.

@treshugart

treshugart May 16, 2018

@developit

This part won't fail, but https://github.com/developit/preact/pull/715/files/3d68b2af246be137c3be8b43f3ea1be84fe59af0#diff-4935bb00e75b7854383877cbfd9d770fR13 would since we new it there.

The worst that can happen at this point, using constructors, is allow it to get to the point of creation to fail because it wasn't registered.

One thing to note here, may be that node.constructor has the possibility of being HTMLUnknownElement if a tag name is used to construct it. This means that if my-custom-element goes unregistered, then <my-custom-element /> is not the same thing as <MyCustomElement />, even though, theoretically they might share the same constructor, you just haven't associated them yet, thus causing it to try and construct the unregistered constructor.

@treshugart

treshugart May 16, 2018

@developit

This part won't fail, but https://github.com/developit/preact/pull/715/files/3d68b2af246be137c3be8b43f3ea1be84fe59af0#diff-4935bb00e75b7854383877cbfd9d770fR13 would since we new it there.

The worst that can happen at this point, using constructors, is allow it to get to the point of creation to fail because it wasn't registered.

One thing to note here, may be that node.constructor has the possibility of being HTMLUnknownElement if a tag name is used to construct it. This means that if my-custom-element goes unregistered, then <my-custom-element /> is not the same thing as <MyCustomElement />, even though, theoretically they might share the same constructor, you just haven't associated them yet, thus causing it to try and construct the unregistered constructor.

This comment has been minimized.

@treshugart

treshugart May 16, 2018

Something we're playing around in Skate with is auto-defining custom elements that aren't defined in a non-destructive way (extending to avoid registration conflicts). See: https://github.com/skatejs/skatejs/blob/master/packages/renderer-preact/src/index.js#L11. You probably don't want to do that here - at least yet - but that's really the only thing we can do besides error with a helpful message that the custom element hasn't been defined yet.

Unfortunately the only good way to check that would be to get some movement on w3c/webcomponents#566. Currently you have to pretty much try / catch, which I'm not sure you want to do in the critical path.

@treshugart

treshugart May 16, 2018

Something we're playing around in Skate with is auto-defining custom elements that aren't defined in a non-destructive way (extending to avoid registration conflicts). See: https://github.com/skatejs/skatejs/blob/master/packages/renderer-preact/src/index.js#L11. You probably don't want to do that here - at least yet - but that's really the only thing we can do besides error with a helpful message that the custom element hasn't been defined yet.

Unfortunately the only good way to check that would be to get some movement on w3c/webcomponents#566. Currently you have to pretty much try / catch, which I'm not sure you want to do in the critical path.

This comment has been minimized.

@developit

developit May 16, 2018

Owner

whoo yeah that's hefty. I'll be keeping an eye on 566...

@developit

developit May 16, 2018

Owner

whoo yeah that's hefty. I'll be keeping an eye on 566...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment