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.

Sign up for GitHub

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

Jump to bottom

CustomElement: make computeProperties() / refreshRendering() report changes to function/class properties too #314

Closed
cjolif opened this issue Oct 14, 2014 · 4 comments
Closed

CustomElement: make computeProperties() / refreshRendering() report changes to function/class properties too #314

cjolif opened this issue Oct 14, 2014 · 4 comments
Assignees
@wkeese
Labels
enhancement
Milestone
0.8.0

Comments

@cjolif
Copy link
Contributor

cjolif commented Oct 14, 2014

This issue was created when I realized changing the itemRenderer property on deliteful List had no effect. itemRenderer property on deliteful List is referencing a default ItemRenderer.

An example is:

<html>
    <head>
        <script src="../requirejs/require.js"></script>
        <script>
            require.config({
                baseUrl: ".."
            });
            require(["delite/register", "dstore/Memory", "delite/Widget"], function(register, Memory, Widget) {
                register("my-widget", [HTMLElement, Widget], {
                    store: Memory,
                    othertype: "test",
                    refreshRendering: function (oldValues) {
                        if ("store" in oldValues) {
                            console.log("I changed store");
                        }
                        if ("othertype" in oldValues) {
                            console.log("I changed othertype");
                        }
                    }
                })
                register.parse();
                foo.store = new Object();
                foo.othertype = "bar";
            })
        </script>
    </head>
    <my-widget id="foo"></my-widget>
</html>

An easy workaround is to not have a default value for the property. But I'm wondering is something can be changed at delite level so people don't have to bother about that.
@wkeese
Copy link
Member

wkeese commented Oct 14, 2014

Hmm, the goal was to avoid watching changes on plain methods like render() and postCreate(), not to avoid watching changes on classes, but I don't know how to differentiate plain methods from classes.

Perhaps this test, but it doesn't cover all cases:

Object.keys(func.prototype).length > 0

We can also detect a dcl() generated class through a hack test like:

Object.keys(func).advices !== undefined

Note that constructor is defined for plain methods too; try typing require("delite/HasDropDown").prototype.render.constructor in the console.

@cjolif
Copy link
Contributor Author

cjolif commented Oct 15, 2014

the goal was to avoid watching changes on plain methods like render() and postCreate(), not to avoid watching changes on classes,

Yes I realized that but then I wondered if in practice this was happening? Do we actually change method implementation? (I mean except if a very rare cases?).

@wkeese
Copy link
Member

wkeese commented Oct 15, 2014

Do we actually change method implementation? (I mean except if a very rare cases?).

No. I skipped functions just to avoid the cost of setting up listeners (which would rarely or never be called).

We could of course try to set up listeners on all widget properties. Maybe the cost is not prohibitive, since they are only setup on the prototype, not on each instance.

@wkeese
Copy link
Member

wkeese commented Oct 31, 2014

The cost is not just the time (and memory) used to create custom setters/getters, but also, every widget method call would first call the custom getter for the method name. I suppose that's no worse than our current accesses to scalar widget properties.

@wkeese wkeese changed the title Widget don't benefit from custom setters & invalidation on properties referencing classes or widgets report changes to function/class properties to computeProperties() / refreshRendering() Jul 7, 2015
@wkeese wkeese changed the title report changes to function/class properties to computeProperties() / refreshRendering() CustomElement: make computeProperties() / refreshRendering() report changes to function/class properties too Jul 7, 2015
@wkeese wkeese mentioned this issue Jul 7, 2015
Closed
@wkeese wkeese self-assigned this Jul 7, 2015
@wkeese wkeese added this to the 0.8.0 milestone Jul 7, 2015
wkeese added a commit to ibm-js/decor that referenced this issue Jul 7, 2015
@wkeese
Make Stateful call custom setters/getters for function properties too.
98d2109 
Also changed the naming of the shadow properties to avoid a conflict where
the getTextDir() property got a shadow property called _getTextDirAttr(),
which looked like a custom getter for the textdir property.
This shouldn't affect existing code assuming it uses this.get("prop")
rather than using the name of the shadow property directly.

Fixes #52 and refs ibm-js/delite#314.
@wkeese wkeese closed this as completed in f6e21cb Jul 7, 2015
wkeese added a commit that referenced this issue Jul 7, 2015
@wkeese
Report updates to function properties too. Fixes #314.
89e1e99
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@wkeese wkeese

Labels
enhancement
Projects
None yet
Milestone
0.8.0
Development

No branches or pull requests
2 participants
@wkeese @cjolif