Skip to content
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

Closed
wants to merge 4 commits into from

Conversation

@treshugart
Copy link

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.

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.

Copy link
@treshugart
@@ -0,0 +1,12 @@
/**

This comment has been minimized.

Copy link
@treshugart
loose: 'all',
blacklist: ['es6.tailCall'],
exclude: 'node_modules/**'
exclude: 'node_modules/**',

This comment has been minimized.

Copy link
@treshugart
loose: 'all',
blacklist: ['es6.tailCall'],
exclude: 'node_modules/**'
exclude: 'node_modules/**',

This comment has been minimized.

Copy link
@treshugart
@@ -17,11 +17,11 @@
"transpile": "npm-run-all transpile:main transpile:devtools transpile:debug",
"optimize": "uglifyjs dist/preact.dev.js -c conditionals=false,sequences=false,loops=false,join_vars=false,collapse_vars=false --pure-funcs=Object.defineProperty --mangle-props --mangle-regex=\"/^(_|normalizedNodeName|nextBase|prev[CPS]|_parentC)/\" --name-cache config/properties.json -b width=120,quote_style=3 -o dist/preact.js -p relative --in-source-map dist/preact.dev.js.map --source-map dist/preact.js.map",
"minify": "uglifyjs dist/preact.js -c collapse_vars,evaluate,screw_ie8,unsafe,loops=false,keep_fargs=false,pure_getters,unused,dead_code -m -o dist/preact.min.js -p relative --in-source-map dist/preact.js.map --source-map dist/preact.min.js.map",
"strip": "jscodeshift --run-in-band -s -t config/codemod-strip-tdz.js dist/preact.dev.js && jscodeshift --run-in-band -s -t config/codemod-const.js dist/preact.dev.js",
"strip": "jscodeshift --run-in-band -s -t config/codemod-strip-tdz.js dist/preact.dev.js && jscodeshift --run-in-band -s -t config/codemod-const.js dist/preact.dev.js && jscodeshift --run-in-band -s -t config/codemod-let-name.js dist/preact.dev.js",

This comment has been minimized.

Copy link
@treshugart
* @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.

Copy link
@treshugart

treshugart May 31, 2017

Author

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
Copy link

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.

src/util.js Outdated
@@ -1,3 +1,5 @@
export const root = typeof window === 'undefined' ? global : window;

This comment has been minimized.

Copy link
@treshugart

treshugart May 31, 2017

Author

I was getting "window is undefined" errors. I couldn't see a convention around this in the source, though.

src/util.js Outdated
/** Coerces and returns a lowercased representation of the argument.
* @param {mixed} str
*/
export function lowercase (str) {

This comment has been minimized.

Copy link
@treshugart

treshugart May 31, 2017

Author

Originally I left everything calling .toLowerCase() on strings. However, in the diffing algorithm, it stringifies the node name and then passes that to a function because the function that gets it doesn't check if it's a string. I actually needed the non-stringified name for a function call after it.

I could have:

  1. Used a different name
  2. Passed the stringified value directly to the function without saving it as a variable

I noticed that several parts of the source was directly calling toLowerCase() on strings. If I could make this into a function that ensures it's a string, then I could remove the need for stringification just to pass of to the function and then be able to use the original value without refactoring much code around it in the differ.

This comment has been minimized.

Copy link
@developit

developit Jun 3, 2017

Member

hmm - this is how preact did lowercasing prior to 8.x, and it was removed for perf/size. need to think on it a bit

This comment has been minimized.

Copy link
@treshugart

treshugart Jun 3, 2017

Author

I can see for perf. Size doesn't make sense because method accesses aren't minified, but function names are. I was trying to offset the amount of bytes added with this. I'll check later when I have a chance.

This comment has been minimized.

Copy link
@developit

developit Jun 3, 2017

Member

Creating a wrapper methods almost always ends up being larger after gzip. Duplicating method/property access is free~ish.

This comment has been minimized.

Copy link
@treshugart

treshugart Jun 5, 2017

Author

Now that I think about it, that makes sense. Good to know!

@@ -10,19 +12,38 @@ export function isSameNodeType(node, vnode, hydrating) {
if (typeof vnode==='string' || typeof vnode==='number') {
return node.splitText!==undefined;
}
if (typeof vnode.nodeName==='string') {
return !node._componentConstructor && isNamedNode(node, vnode.nodeName);
const { _componentConstructor } = node;

This comment has been minimized.

Copy link
@treshugart

treshugart May 31, 2017

Author

These were referenced in several places, so I just saved them as constants so they get reused instead of accessing the object several times.

This comment has been minimized.

Copy link
@developit

developit Jun 3, 2017

Member

ends up being larger :(
also hoisting this means it gets accessed even when hydrating is true, which is a perf hit.

This comment has been minimized.

Copy link
@treshugart

treshugart Jun 3, 2017

Author

Fair enough. I'll revert these changes here, also because it stays withinin convention.

* @param {Element} node
* @param {mixed} nodeName
*/
export function isSameNodeName(node, nodeName) {

This comment has been minimized.

Copy link
@treshugart

treshugart May 31, 2017

Author

After adding isSameNodeConstructor (to be consistent with isSameNodeType, I felt that this should also be made consistent. I also couldn't understand the previous name as easily.

@@ -0,0 +1,18 @@
/* eslint-disable */

This comment has been minimized.

Copy link
@treshugart

treshugart May 31, 2017

Author

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.

Copy link
@treshugart

treshugart May 31, 2017

Author

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.

Copy link
@treshugart

treshugart May 31, 2017

Author

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.

Copy link
@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.

Copy link
@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).

"size": "node -e \"process.stdout.write('gzip size: ')\" && gzip-size dist/preact.min.js",
"test": "npm-run-all lint --parallel test:mocha test:karma test:ts",
"test:ts": "tsc -p test/ts/",
"test:mocha": "mocha --recursive --compilers js:babel/register test/shared test/node",
"test:mocha": "mocha --recursive --require babel-register test/shared test/node",

This comment has been minimized.

Copy link
@xtuc

xtuc Jun 1, 2017

👍 babel/register is deprecated

This comment has been minimized.

Copy link
@treshugart

treshugart Jun 1, 2017

Author

@xtuc This is probably coming in from #683 as I had to base it off that for proper class transpilation.

src/util.js Outdated
/** Coerces and returns a lowercased representation of the argument.
* @param {mixed} str
*/
export function lowercase (str) {

This comment has been minimized.

Copy link
@developit

developit Jun 3, 2017

Member

hmm - this is how preact did lowercasing prior to 8.x, and it was removed for perf/size. need to think on it a bit

@@ -10,19 +12,38 @@ export function isSameNodeType(node, vnode, hydrating) {
if (typeof vnode==='string' || typeof vnode==='number') {
return node.splitText!==undefined;
}
if (typeof vnode.nodeName==='string') {
return !node._componentConstructor && isNamedNode(node, vnode.nodeName);
const { _componentConstructor } = node;

This comment has been minimized.

Copy link
@developit

developit Jun 3, 2017

Member

ends up being larger :(
also hoisting this means it gets accessed even when hydrating is true, which is a perf hit.

return !node._componentConstructor && isNamedNode(node, vnode.nodeName);
const { _componentConstructor } = node;
const { nodeName } = vnode;
if (typeof nodeName==='string') {

This comment has been minimized.

Copy link
@developit

developit Jun 3, 2017

Member

I think you actually want the custom element check here, otherwise all custom elements are treated as the same element during hydration, or always as a mismatch.

This comment has been minimized.

Copy link
@treshugart

treshugart Jun 3, 2017

Author

You may be right. I'll check.

* @param {Element} node
* @param {Function} nodeName
*/
export function isSameNodeConstructor({ constructor }, nodeName) {

This comment has been minimized.

Copy link
@developit

developit Jun 3, 2017

Member

i know this sounds crazy, but preact doesn't contain any destructuring as a rule. it makes the output larger and hides a property access that often need be safeguarded. In this particular case it's likely a lot faster to inline .constructor===nodeName rather than pass many object types to a test function.

This comment has been minimized.

Copy link
@treshugart

treshugart Jun 3, 2017

Author

Cool. I'll change that.

@developit
Copy link
Member

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
Copy link
Author

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
Copy link
Author

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
Copy link
Member

developit commented Jun 3, 2017

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

@developit
Copy link
Member

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
Copy link

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
Copy link
Author

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
Copy link

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.

- 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
@treshugart treshugart force-pushed the treshugart:master branch from 4189168 to c90d907 Jun 6, 2017
@coveralls
Copy link

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
Copy link

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
Copy link

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.

Copy link
@developit

developit May 14, 2018

Member

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

Sorry for the delays on this PR.

This comment has been minimized.

Copy link
@treshugart

treshugart May 16, 2018

Author

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

Copy link
@treshugart

treshugart May 16, 2018

Author

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.

Copy link
@developit

developit May 16, 2018

Member

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

@treshugart
Copy link
Author

treshugart commented Nov 15, 2019

I'm going through some of my old PRs and found this old gem. This is now a couple of years old so probably safe to close :). Let me know if you feel like this should go into latest and I can do some work on it.

@treshugart treshugart closed this Nov 15, 2019
@andrewiggins
Copy link
Member

andrewiggins commented Nov 15, 2019

FWIW, I believe Preact X has full support for custom elements :) If someone comes across this and discovers a bug with custom elements in Preact X, file a new issue so we can look into it!

@treshugart
Copy link
Author

treshugart commented Nov 15, 2019

@andrewiggins even supporting a <CustomElement /> constructor? If so, that's rad (I haven't used Preact in awhile).

@andrewiggins
Copy link
Member

andrewiggins commented Nov 16, 2019

Oh sorry, I misunderstood what this PR was adding. Preact still won't register Custom Elements for you or accept them in the constructor of VNodes. My mistake. (May be possible to add as a VNodes option if that wasn't already suggested though)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants
You can’t perform that action at this time.