ibm-js / delite

HTML Custom Element / Widget infrastructure
http://ibm-js.github.io/delite/
Other
68 stars 28 forks source link

test or remove delite/Store code for when store prop is custom element #397

Closed wkeese closed 9 years ago

wkeese commented 9 years ago

delite/Store has the following code:

if (!this.store.filter && this.store instanceof HTMLElement && !this.store.attached) {
    // this might a be a store custom element, wait for it
    this.store.addEventListener("customelement-attached", this._attachedlistener = function () {
        this.queryStoreAndInitItems(this.processQueryResult);
    }.bind(this));

That code is not needed, or at least, the regression tests for delite run fine even if it's removed. So the code should either be removed, or a regression test added.

Also:

  1. The check for this.store.filter seems weird. I don't see why that should be there:
if (!this.store.filter && this.store instanceof HTMLElement && !this.store.attached) {
  1. It would be cleaner to disconnect the listener like this:
this.store.addEventListener("customelement-attached", this._attachedlistener = function () {
    this.store.removeEventListener("customelement-attached", this._attachedlistener);
    this.queryStoreAndInitItems(this.processQueryResult);
}.bind(this));

@cjolif?

cjolif commented 9 years ago

That code is for deliteful/Store. I suspect deliteful tests might start failing if you remove it. I guess the only way to test it in delite is to replicate deliteful/Store feature in delite itself.

Remark 2 clearly make sense. For 1 I guess I would have to think more about it. Maybe I wanted to cover some non obvious cases but you are right this is not obvious.

wkeese commented 9 years ago

Actually, this code is all gone as of 56672f8fd794771cf95c0229aee15e206d530394, I guess because that commit gets rid of the <d-store> type custom tag and embeds data directly within the widget (for example <d-list>.