Support for custom elements #121

diffcunha · 2018-03-21T20:28:56Z

This PR adds the support for custom components (web components)

// MyComponent.js

class MyComponent extends HTMLElement {
  connectedCallback () {
    this._shadow = this.attachShadow({ mode: 'open' })
    this._shadow.appendChild(html`<span>Hello ${this.getAttribute('foo')}</span>`)
  }
}

MyComponent.tagName = 'my-component'

// MyParagraph.js

class MyParagraph extends HTMLParagraphElement {
  connectedCallback () {
    this._shadow = this.attachShadow({ mode: 'open' })
    this._shadow.appendChild(html`<strong>${this.children[0]}</strong>`)
  }
}

MyParagraph.tagName = 'my-paragraph'
MyParagraph.options = { extends: 'p' }

// index.js

var tree = html`
  <div>
    <${MyComponent} foo="bar"></${MyComponent}>
    <p is=${MyParagraph}><span>Lorem ipsum</span></p>
  </div>
`

bcomnes · 2018-03-22T02:08:08Z

Very cool!

yoshuawuyts · 2018-03-22T05:44:38Z

lib/browser.js

+  if (props.is) {
+    if (typeof props.is === 'function') {
+      if (!customElements.get(props.is.tagName)) {
+        customElements.define(props.is.tagName, props.is, props.is.options)


I'm not feeling great about this line - the spec really just wants you to register elements once. This would register an element on every call.

It only registers if it isn't register yet if (!customElements.get(props.is.tagName))

yoshuawuyts · 2018-03-22T05:46:00Z

I'm open to landing this, but in order to do so it would need to include tests for client, server & transforms too. It's a pretty big patch, and I suspect there might be a few rough edges.

diffcunha · 2018-03-23T12:58:19Z

@yoshuawuyts PR updated, only babel transform is missing. @goto-bus-stop and @AndyOGo pushed a few commits replacing bel with nanohtml on tests. I was doing the same so there might be some weird things to be fixed

AndyOGo · 2018-03-23T13:30:26Z

@diffcunha Correct I refactored all tests, which where using bel instead of nanohtml.
I would suggest you pull and rebase your branch onto master to get the updated stuff.

diffcunha · 2018-03-23T14:30:16Z

@AndyOGo I rebased already. I have a question: how did you made the require('nanohtml') work in tests? Previously bel was available to be required because it’s a dep of choo and choo is a dep of this project. I had to use pathmodify plugin to provide nanohtml as a relative path.

AndyOGo · 2018-03-23T14:32:28Z

@diffcunha That's weird. Actually I did nothing... just run npm run test to check if it works, and it worked 🤔

goto-bus-stop · 2018-03-23T14:32:42Z

@diffcunha the transform output didn't use the main nanohtml import previously, so it didn't matter whether the import path could be resolved.

diffcunha · 2018-03-23T14:34:20Z

@goto-bus-stop that’s it! Thanks

diffcunha · 2018-03-23T15:24:05Z

tests/transform/index.js

-  })
-})
-
-test('xlink:href', function (t) {


These tests (svg and xlink:href) were removed since the createElement function is now responsible for handling those and not the transform itself

diffcunha · 2018-03-26T04:35:47Z

tests/babel/fixtures/dynamicAttr.js

@@ -11,9 +11,9 @@ html`
  <div id="str" ${className}="blub" />
 `

-var x = 'disabled'
+var x = { disabled: 'disabled' }
 html`
  <button ${x} id="lol" />


I'm not sure if this is supposed to be allowed as it creates ambiguity with props spread

var obj = { prop: 'value' } var tree = html`<button ${obj}>foobar</button>`

There is also some inconsistency within the previous implementations:

var x = 'disabled' var tx = html`<button ${x}>foobar</button>` // tx.outerHTML == <button disabled="disabled">foobar</button> var y = false var ty = html`<button disabled=${x}>foobar</button>` // ty.outerHTML == <button>foobar</button> tx.outerHTML !== ty.outerHTML

Regardless, I can spend some time making prop spread and this shorthand notation to work together

It is supposed to be allowed, yes

diffcunha · 2018-03-26T04:47:20Z

@yoshuawuyts PR updated. The major change here is that transformed code is generated around our createElement only instead of document.createElement and other auxiliary functions. One single implementation rather than 3 separate ones (browserify transform, babel plugin and template string runtime eval)

goto-bus-stop · 2018-03-26T15:42:56Z

Had to take some time to get used to this massive refactor, but I like the createElement change for sure 👍 appreciate your work on this!

goto-bus-stop

some comments and q's. thanks!

goto-bus-stop · 2018-03-28T10:02:38Z

lib/createElement.js

+    if (typeof props.is === 'function') {
+      if (!window.customElements.get(props.is.tagName)) {
+        window.customElements.define(props.is.tagName, props.is, props.is.options)
+      }


I don't think the renderer should be in charge of defining custom elements. I'm also worried about introducing nonstandard static properties on HTML element classes but it doesn't seem like there's a way around that. Having Component.tagName is fine, but IMO automagic registration and Component.options is not nanohtml's business

OK. In fact the tagName prop only serves the purpose of registration. I understand that registration shouldn't not be a concern of nanohtml, it will be then up to the dev to register the components before rendering

Sure registration is out of scope from nanohtml

goto-bus-stop · 2018-03-28T12:59:38Z

lib/browserify-transform.js

+  objectProperty: function (key, value, computed) {
+    return computed
+      ? ('[' + key + ']' + ':' + value)
+      : (key + ':' + value)


choo aims to work in IE 11 + so we can't use ES6+ syntax in the transform output

OK, will fix that

goto-bus-stop · 2018-03-28T13:02:04Z

lib/browserify-transform.js

+    return html + '.createElement(' + tag + ',' + props + ',' + children + ')'
+  },
+  callObjectAssign (objects) {
+    return 'Object.assign(' + objects.join(',') + ')'


Object.assign also doesn't exist yet in IE11 … choo uses the xtend module, that might be good (deduped = no extra bytes 🎉 ). maybe xtend can be exposed as nanohtml/lib/merge or so, and that can be required in the output, that way we don't add a peer dependency.

IE 11 will probably be dropped in choo 7 but not yet: choojs/choo#616

OK, will use xtend instead

goto-bus-stop · 2018-03-28T13:08:47Z

lib/browserify-transform.js

    if (isSupportedView(node)) {
      if (node.arguments[0].value === 'bel' ||
        node.arguments[0].value === 'choo/html' ||
        node.arguments[0].value === 'nanohtml') {
        // html and choo/html have no other exports that may be used
-        node.edit.update('{}')
+        node.edit.update('{ createElement: require("' + path.join(node.arguments[0].value, '/lib/createElement") }'))


would it make sense to just do update('require("/path/to/createElement")') here and call that function directly as html() or createElement() (not renaming the variable is probably easier) instead of doing html.createElement?

choo/html is not going to have an export named choo/html/lib/createElement, and bel didn't have one either. maybe this should use require.resolve('./createElement') instead? or path.relative() from the input file

Yeah, that was my first idea, but then I thought it wouldn't be a good idea to "reuse" an already existent api. I mean, one might want to:

var html = require('nanohtml') var foo = function (strings, ...keys) { // do stuff return html(strings, ...keys) } var bar = html`<span>HELLO</span>`

You are absolutely right about bel and choo/html. Regarding choo/html, I could still replace with nanohtml/lib/createElement or something like it. Regarding bel, I'm not so sure but probably it won't be supported in this version on nanohtml given that it doesn't expose a createElement function

goto-bus-stop · 2018-03-28T13:11:08Z

lib/createElement.js

@@ -0,0 +1,98 @@
+var appendChild = require('./append-child')


small nit: the other files use dashes rather than camelcase

goto-bus-stop · 2018-03-28T13:13:04Z

lib/transform.js

+  var aexpr
+  var bexpr
+
+  if (iscexpr(a)) aexpr = a // c-expression


what's the c in c-expression?

c-expression as in concat-expression. They should be probably called meta-expressions since that what they are

goto-bus-stop · 2018-03-28T13:16:43Z

tests/transform/index.js

  b.bundle(function (err, src) {
    fs.unlinkSync(FIXTURE)
    t.ifError(err)
-    t.ok(src.indexOf('document.createElement("div")') !== -1, 'created a tag')
+    t.ok(src.indexOf('html.createElement("div",{"class":"whatever "+abc},[xyz])') !== -1, 'created a tag')


to only test if the tag is being created checking html.createElement("div", would be good but, not a big deal

goto-bus-stop · 2018-03-28T13:17:50Z

tests/transform/index.js

    transform: path.join(__dirname, '../../')
  })
+  // don't b.require createElement, it will hang this test


oops this is probably this browserify bug: browserify/browserify#1707 looks like that might still be an issue after all

goto-bus-stop · 2018-03-28T13:25:31Z

lib/babel.js

+                })
+                path.unshiftContainer('body', t.importDeclaration([
+                  t.importDefaultSpecifier(createElement)
+                ], t.stringLiteral(library + '/lib/createElement')))


I think we should just always output nanohtml/lib/createElement in the babel transform, only nanohtml exposes that. this means that transformed code will have a peer dependency on nanohtml, but that was also a limitation in the yo-yoify babel plugin.

goto-bus-stop · 2018-03-28T13:26:06Z

lib/browserify-transform.js


-module.exports = function yoYoify (file, opts) {
+var SUPPORTED_VIEWS = ['nanohtml', 'bel', 'yo-yo', 'choo/html']


good, choo.view is long gone afaik 👍

jmealo · 2018-07-31T20:13:56Z

@goto-bus-stop @diffcunha: Kudos on the team work! Where did we land on this?

AndyOGo · 2018-11-06T13:47:56Z

@diffcunha
@goto-bus-stop
@bcomnes
@jmealo
I just submitted a relatively smaller PR #137 , which just adds support for is attribute (extended build-in elements as defined by custom elements V1 spec), please see #136

I think that should be all which is needed.
May I ask you to review it?

AndyOGo · 2018-11-06T16:49:15Z

PS: I also provided tests 💪

yoshuawuyts reviewed Mar 22, 2018

View reviewed changes

diffcunha added 3 commits March 23, 2018 12:53

Support for custom elements

e90ec6e

eslint fix

62fe714

new browserify transform and tests fixed (babel missing)

5a5c31d

diffcunha commented Mar 23, 2018

View reviewed changes

diffcunha added 2 commits March 23, 2018 15:55

small fix and more tests

1895e41

pathmodify removed

2bf9f8b

diffcunha commented Mar 26, 2018

View reviewed changes

Single transform abstraction

8048e07

diffcunha added 2 commits March 26, 2018 16:14

set-attribute.js removed

7695232

deps cleanup

02dbc74

type def updated

5589e24

goto-bus-stop requested changes Mar 28, 2018

View reviewed changes

goto-bus-stop mentioned this pull request Nov 16, 2018

Feature/add custom element built in support #137

Merged


		module.exports = function yoYoify (file, opts) {
		var SUPPORTED_VIEWS = ['nanohtml', 'bel', 'yo-yo', 'choo/html']

Support for custom elements #121

Are you sure you want to change the base?

Support for custom elements #121

Conversation

diffcunha commented Mar 21, 2018 • edited

bcomnes commented Mar 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yoshuawuyts commented Mar 22, 2018

diffcunha commented Mar 23, 2018

AndyOGo commented Mar 23, 2018 • edited

diffcunha commented Mar 23, 2018 • edited

AndyOGo commented Mar 23, 2018

goto-bus-stop commented Mar 23, 2018 • edited

diffcunha commented Mar 23, 2018

Choose a reason for hiding this comment

diffcunha Mar 26, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diffcunha commented Mar 26, 2018

goto-bus-stop commented Mar 26, 2018

goto-bus-stop 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

diffcunha Apr 3, 2018 • edited

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

jmealo commented Jul 31, 2018

AndyOGo commented Nov 6, 2018 • edited

AndyOGo commented Nov 6, 2018

diffcunha commented Mar 21, 2018 •

edited

AndyOGo commented Mar 23, 2018 •

edited

diffcunha commented Mar 23, 2018 •

edited

goto-bus-stop commented Mar 23, 2018 •

edited

diffcunha Mar 26, 2018 •

edited

diffcunha Apr 3, 2018 •

edited

AndyOGo commented Nov 6, 2018 •

edited