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

regression in build for templates with embedded custom elements #438

Closed
wkeese opened this issue Feb 19, 2016 · 1 comment
Closed

regression in build for templates with embedded custom elements #438

wkeese opened this issue Feb 19, 2016 · 1 comment
Assignees
Labels
Milestone

Comments

@wkeese
Copy link
Member

wkeese commented Feb 19, 2016

Templates with embedded widgets no longer work correctly in a built layer. For a template like below, where <my-embedded-widget> has a property called foo:

<template requires=acme/MyEmbeddedWidget>
    <my-embedded-widget foo=bar>
    ...

... the generated code is calling setAttribute():

define("delite/handlebars!acme/MyCompoundWidget.html", [ "acme/MyEmbeddedWidget" ], function() {
    return function anonymous(document, register) {
        var c1 = register.createElement("my-embedded-widget");
        c1.setAttribute("foo", "bar");

But, it should be setting the property directly:

c1.foo = "bar";

(Note that this wouldn't be a critical issue if we had attribute reflection as mentioned in #33. But we don't. And even if we did, it would still be somewhat of a performance issue.)

This is a regression from #432.

@wkeese wkeese self-assigned this Feb 19, 2016
@wkeese wkeese added this to the 0.8.2 milestone Feb 19, 2016
@wkeese
Copy link
Member Author

wkeese commented Feb 19, 2016

For Template.js to work properly, it needs to instantiate a dummy copy of <my-embedded-widget> and introspect what the native properties are. This isn't happening during a build. Furthermore, it's a bit problematic to do during a build, because that requires that all the custom element code (i.e. MyEmbeddedWidget.js) load correctly on Node. This doesn't happen naturally because symbols like HTMLElement aren't defined in Node. You can get a definition of HTMLElement from the "jsdom" package but it seems like an uphill battle.

The compilation of template HTML into the JSON-esque AST (as documented on http://ibm-js.github.io/delite/docs/0.8.1/Template.html) works fine on the server. The problem is the second stage, compiling down to javascript. That second stage is also somewhat questionable since the footprint of a template in javascript form is bigger than the footprint in AST form. I think in this case discretion is the better part of valour and it makes sense for the build to just compile down to the AST.

Another possible change would be to rewrite Template.js to not do code generation, but rather instead to render from an optimized version of the AST, something like:

{
    // attributes to set, like this.setAttribute("class", ...)
    attributes: {
        className: {
                      value: function () { return "d-reset" + this.baseClass },
                      dependsOn: ["baseClass"]
                },
        ...
    },

    // properties to set, like this.foo = 123
    properties: {
        className: {
                      value: function () { return this.bar },
                      dependsOn: ["bar"]
                },
        ...
    },
    ...
}

Again, that would be a trade off between the performance of instantiating a template vs. the code size of Template.js.

@wkeese wkeese closed this as completed in 1a3b5b6 Feb 19, 2016
wkeese added a commit that referenced this issue Mar 11, 2016
Turns out that it's problematic even to create the AST at build time, because then
(during the build) handlebars.js will create dummy custom elements (ex: <d-button>)
for introspection.  They are used to find out when to call element.foo = false
rather than element.foo = "false".

Creating the dummy elements first requires running the code to define the custom elements.
That wasn't working right during builds, because the javascript files were
referencing symbols that don't exist in Node, such as HTMLElement.

Rolls back more of (and most of) 2c0633f.

Fixes #438 for real, and refs #432.
@wkeese wkeese added the bug label Apr 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant