Support for attributes, type-extension CEs, and those loaded by HTML import

[rnicholus]

...I also added an "engines" property to package.json to reflect the earliest version of Node.js required to run the unit test suite, which appears to be 6+. No changes I made here influenced the required version of Node to run the tests, this was a pre-existing requirement. The "engines" property provides documentation of this requirement if nothing else.

Note: There are no breaking changes in this PR, only additions. The updated version number reflects this.

Background

I am personally a fan of HTML imports. While I understand this portion of the Web Components specification is controversial in some circles, I love the way this portion of the spec provides for standardized self-contained modularization of HTML, JS, and CSS for web components. I can easily define the HTML, CSS, and JS for my Web Component in appropriate files and tie them all together in a way that is transparent to the user, which simply imports the root HTML file using a single HTML import. While this certainly will increase the number of HTTP requests, HTTP/2 (supported in all modern browsers) mitigates this with connection multiplexing.

I tend to use HTML imports often when I write my own web components. A couple OSS WCs I wrote that utilize HTML imports include ajax-form and file-input. But this is not an uncommon pattern, so I quickly ran into trouble when attempting to easily integrate WCs into my React-based projects. For the most part, webcomponents/react-integration makes this possible for WCs where the constructor function is easily accessible, but more work was required to properly Reactify WCs that relied on HTML imports. My work to support these types of WCs includes the commits in this pull-request as well as a Webpack loader I wrote that provides easy access to the "root" URL of an HTML-importable WC.

Changes in this PR

My pull request provides support for the following:

1. Associating attributes with custom elements.
2. Reactifying a custom element given only its tag name.
3. Reactifying all type-extension custom elements.

The remainder of this PR description goes into more detail regarding the above three items.

Associating attributes with custom elements

In a perfect world, all element properties would be automatically reflected as attributes where appropriate. But there are a number of custom elements in existence that do not properly update attributes when property changes occur. I have come across many of these myself, which is what prompted some of the changes in this pull request. Admittedly, I'm also the author of a few such CEs. While the long-term goal is to "fix" these CEs such that they sync property changes to attributes whenever prudent, it is desirable to allow direct access to attributes at least until these CEs can be updated (if possible).

My pull request looks for properties that start with attr-, and passes the associated value directly to the CE as an attribute. For this type of property, object/array values are also stringified before setting the element attribute. There are corresponding unit tests to verify this new behavior.

Reactifying a custom element given only its tag name

This is especially important for web components that are loaded via HTML import. In those instances, the constructor can be accessed, but the process may not be intuitive. Most importantly, the CE may not be fully "created" at the time the React component wrapper is rendered. In this case, it is far easier to simply pass the tag-name. This cuts out the code that uses the CE constructor to determine the tag-name, which itself expects the CE to be fully created at that point.

You can see the changes I made to the index file along with a number of new unit tests. I also took care to update the Typescript definitions file to reflect the fact that either a CE constructor function or a tag-name (string) may be passed as the first parameter to the exported function.

Reactifying all type-extension custom elements

This one was pretty simple to address. If a property named is is passed to the reactified CE, ensure this value is set on the first render of the element. Once the element has been added to the DOM, specifying the is property doesn't seem to have any affect. This is not a React-specific issue, though rendering the CE initially without the is property produces the same problem.

I've also updated the README.md file to include documentation for all newly supported features.

[treshugart]

This is awesome @rnicholus, thank you! I've had limited availability, so hopefully I'll get to reviewing this soon. Sorry for the delay :(

[rnicholus]

No rush. I know there's a lot to digest here. Let me know if I can assist in any other way.

Aside: While this library with my changes + the Webpack loader addresses most of the issues I had while trying to integrate web components into a React app, one problem I haven't yet solved is how to get HTML imports and ES6 imports to play nicely. How can we de-dupe across both of these mechanisms? I've been giving that some thought, but have not come up with a good answer yet. In the meantime, I believe this is a step in the right direction.

[treshugart]

Re the aside, you might be able to solve it with a Webpack plugin that checks hashed content. Not sure, though, I haven't given it much thought either.

[rnicholus]

My fear is that the two just won't be able to work together. It really has to be one or the other in terms of de-duping. I could update my Webpack plug-in to rip out all the <script src=...> in the WC's HTML files and instead feed those through Webpack. This would mean they are included alongside all other JS resources in the generated bundle(s). I'm not 100% sure that will actually solve the problem though, and in some cases the <template> tags need to be present in the DOM before the corresponding WC JS is executed.

At this point, I'm not worrying too much about loading duplicate resources. In many cases, this probably won't be a big deal, until a project mixes a great number of HTML-imported WCs and ES6 imported code/WCs.

[treshugart]

Hey @rnicholus sorry it took so long for me to get around to reviewing this.

[rnicholus]

No worries, nothing pressing here. It may be a day or so before I can get around to responding to your comments anyway, as I'm working through some other side project commitments over the next few weeks.

[treshugart]

PR to implement customised built-ins in the CE pfill: https://github.com/webcomponents/custom-elements/pull/42.

[rnicholus]

PR to implement customised built-ins

Speak of the devil. Looks like that was created w/in the last hour.

[treshugart]

Someone else was working on converting this over to v1. Maybe we should help finish that off then polish this up. Depends on #59 which is being done as part of $58 in https://github.com/kentendo/react-integration.

[rnicholus]

Would love to help out. Is there a timeline for that to be completed?

off-topic: Congrats on Trello. We use JIRA + Confluence at my company. Will be interesting to see how putting all of those under one roof benefits users.

[treshugart]

@rnicholus re timeline, not sure if open source has one :). Availability + ASAP haha. It'd be great if you wanted to help out. Cheers!

[treshugart]

Hey @rnicholus I just wanted to give this a bump to see where it's at. Totes understand that you've probably been really busy. Very excited for this to go in!

[rnicholus]

Thanks for the reminder. It fell off my radar, but I'll add it to my task list so that does not happen again. I'll start by rereading your comments in the next day or so and take it from there

[rnicholus]

@treshugart I +1'd a few of your suggestions. Please let me know if there are others that you feel strongly about. Otherwise, I'll begin updating my PR with those changes post-haste.

One thing: my thoughts on custom elements have evolved a bit since I created this PR. I see the v1 spec as the future (and of course it is), and am more willing to leave behind html imports as a result. It seems possible that the HTML imports spec will be deprecated at some point, unless Firefox, Safari, and Edge choose to implement this part of the WC spec. It looks like Safari and Firefox will never implement though. So, I'm not sure if this PR will do anything more than clutter up your library.

...but maybe the is property portion of my PR is still valuable. That's a fairly trivial change though.

[treshugart]

@rnicholus cool, it's kinda hard for me to tell which ones weren't thumbs up'd be fore, but looks good. I can't tell from the thumbs up if you're on board with auto-applying the is attribute, re the reactify('ajax-form') stuff. What's your stance on that?

[rnicholus]

Re: using reactify('ajax-form'), it sounds like that would require using customElements.get() to get a handle on the constructor. If HTML imports are involved, the call may return undefined if the HTML imports hasn't completed yet. But if you aren't looking to support HTML imports, that may be a moot point.

[treshugart]

There's an API in the polyfill to detect when all imports have loaded and the consumer could reactify after that. That said, I don't think it's a good idea to support very contentious standards that aren't yet finalised. As long as the consumer has the control over when they call reactify() we should be future proof.

[rnicholus]

Makes sense. Do you think it is still important to allow users to pass tag names, instead of CE constructor functions.

[treshugart]

Maybe not? I don't have strong feelings about that other than it's easier to add than it is to subtract. Meaning, since the spec hasn't stabilised around HTML Imports, it might be better to wait for a bit before adding that functionality.

If this is going to block you from using it, then I think it's valuable to have that API if it's necessary. I'll let you make the judgement call here :)

[rnicholus]

If this is going to block you from using it

It may, without HTML import support, but I've realized that I need to embrace v1 CEs, and have already started re-writing one of my CEs to account for this.

One thing I don't know the answer to yet: What tag name will be used to render the CE by react-integration if something like reactify('ajax-form') is used? Will the tagName be 'form', or 'ajax-form'? I would think it would have to be 'form', since the tag is a <form>, not an <ajax-form>, but I'm not sure what the value of tagName would be on the constructor function to be honest. If it is instead 'form', then the user still needs to set an is prop, and we still need to be sure is is added on the initial render AFAIK.

[treshugart]

If we left it only as a constructor, you can use the localName and the is attribute from the constructed, temporary, element to pass that on to the virtual element without requiring a string as an argument. The only potential problem is that the element may not have been defined yet. I don't think that's a problem, though, as that responsibility should be put onto the consumer to ensure that they call reactify(Ctor) after the constructor is defined. If they know the tag name, they can also use the customElements.whenDefined(ceName) API to do this.

[rnicholus]

Ok, so I plan to do the following (let me know if I've missed anything, or included something redundant):

• [x] Remove all code that was put in place specifically to support HTML imports.
• [ ] Auto-apply is for type extension CEs (as discussed above).
• [x] attrs property w/ an object value that holds all attributes to apply to the element.
• [ ] Final docs update based on these changes.

[treshugart]

I do think it's important to solve your use-cases, so if we find that we do eventually require using strings, then I'm happy to discuss it at that point.

I created #65 to add an events prop just like attributes in a future PR.

One last bikeshed, what do you think about using attrs instead of attributes to match the props nomenclature? I don't have strong opinions either way, but it's worth considering at this point. I'll let you make that call.

Happy with what you've proposed in your last comment. Thank you!! :)

[rnicholus]

what do you think about using attrs instead of attributes to match the props nomenclature?

fine by me - just updated my checklist

[rnicholus]

At the moment, I'm blocked on the "automatically add the is property" task. A couple problems:

First, constructing a CE that extends another element results in a TypeError. For example:

class XFoo extends HTMLSpanElement {
   constructor() {
      super()
   }
}

customElements.define('x-foo', XFoo, {extends: 'span'})

// Uncaught TypeError: Illegal constructor
new XFoo()

Also, I'm not certain how to determine both the tagName ('SPAN') and the is value to apply to the element ('x-foo') given only the passed constructor. Maybe I'm missing something, but it doesn't seem like we can determine the proper value to set for is with nothing more than a constructor function.

[rnicholus]

After further research, even though extends was recently added to the spec, Chrome doesn't seem to support this yet. I think it's wise to hold off on this until at least the v1 polyfill has been updated. My second problem (above) remains though.

[treshugart]

I think it's wise to hold off on this until at least the v1 polyfill has been updated. My second problem (above) remains though.

Which part exactly? It seems that both have to do with customised built-ins. If the advice is to hold off, then wouldn't it be worth holding off on that one, too? Maybe I misunderstood.

[rnicholus]

I plan to hold off on the auto-"is" feature until the polyfill matches the spec. But even after the polyfill is updated, a question regarding how this can be completed remains. That question/issue, as posed earlier, is:

I'm not certain how to determine both the tagName ('SPAN') and the is value to apply to the element ('x-foo') given only the passed constructor. Maybe I'm missing something, but it doesn't seem like we can determine the proper value to set for is with nothing more than a constructor function.

[treshugart]

I'm not certain how to determine both the tagName ('SPAN') and the is value to apply to the element ('x-foo') given only the passed constructor. Maybe I'm missing something, but it doesn't seem like we can determine the proper value to set for is with nothing more than a constructor function.

Well, we currently construct the element to get information about it. Polyfill / native will automatically apply the is attribute, so we can use localName and getAttribute('is') (or maybe the is prop?) to get both pieces of information.

Alternatively, a better solution might be to override customElements.define() to store the information so we don't have to construct the element just to get the information.

That should work, no?

[treshugart]

That said, we probably don't need to do that until the polyfill supports is, correct? So can we merge this?

[treshugart]

Ping :). If this is gtg now, happy to merge.

[rnicholus]

Sorry for the delay. I don't think this is ready to merge. I was unable to complete/verify support for built-in support due to the lack of native support for built-ins in any browser and the lack of support in the polyfill. Perhaps that has changed recently or there is a workaround, but I'm unable to look into this further now. I'm hoping to get some time this month to take another look.

[treshugart]

@rnicholus okay, no worries. Just following up. Let me know when you need me to take another look :)

[treshugart]

Closing because stale. LMK if there's ever any intention to update, and thank you so much for the original PR.