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

Support for custom elements #121

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Support for custom elements #121

wants to merge 9 commits into from

Conversation

diffcunha
Copy link

@diffcunha diffcunha commented Mar 21, 2018

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

Blocked on: choojs/hyperx#68

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

bcomnes commented Mar 22, 2018

Very cool!

lib/browser.js Outdated
if (props.is) {
if (typeof props.is === 'function') {
if (!customElements.get(props.is.tagName)) {
customElements.define(props.is.tagName, props.is, props.is.options)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@yoshuawuyts
Copy link
Member

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

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

AndyOGo commented Mar 23, 2018

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

diffcunha commented Mar 23, 2018

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

AndyOGo commented Mar 23, 2018

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

@goto-bus-stop
Copy link
Member

goto-bus-stop commented Mar 23, 2018

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

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

})
})

test('xlink:href', function (t) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

var x = 'disabled'
var x = { disabled: 'disabled' }
html`
<button ${x} id="lol" />
Copy link
Author

@diffcunha diffcunha Mar 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is supposed to be allowed, yes

@diffcunha
Copy link
Author

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

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!

Copy link
Member

@goto-bus-stop goto-bus-stop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some comments and q's. thanks!

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure registration is out of scope from nanohtml

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, will fix that

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, will use xtend instead

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") }'))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@diffcunha diffcunha Apr 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small nit: the other files use dashes rather than camelcase

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha

var aexpr
var bexpr

if (iscexpr(a)) aexpr = a // c-expression
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the c in c-expression?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good, choo.view is long gone afaik 👍

@jmealo
Copy link

jmealo commented Jul 31, 2018

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

@AndyOGo
Copy link
Contributor

AndyOGo commented Nov 6, 2018

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

AndyOGo commented Nov 6, 2018

PS: I also provided tests 💪

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

Successfully merging this pull request may close these issues.

6 participants