`attributeChangedCallback()` bound value is invalid

[kamituel]

In the CERDefine[ATTRIBUTE_CHANGED_CALLBACK], the following is used to invoke the attributeChangedCallback():

CProto[ATTRIBUTE_CHANGED_CALLBACK].apply(this, arguments);

If I understand the spec correctly, the this should be bound to the element instance instead:

Invoke reaction's callback function with reaction's arguments, and with element as the callback this value.

(emphasis mine)

Testing this in real browsers shows that the this in the attributeChangedCallback() is indeed bound to the instance element.

I saw the V1 Caveat in the README.md, as well as #168 , but unsure if both are related to this issue. Is it? Or is it a bug and a proper PR with a fix would be accepted?

So the following would be a quick fix:

    safeProperty(proto, CREATED_CALLBACK, {
        value: function () {
          if (justCreated) justCreated = false;
          else if (!this[DRECEV1]) {
            this[DRECEV1] = true;
            // -------- FIX ----------------
           //  (Store the reference to the instance)
            this.__dreInstance = new Class(this);
            // -------- END FIX ----------------
            if (CProto[CREATED_CALLBACK])
              CProto[CREATED_CALLBACK].call(this);
            var info = constructors[nodeNames.get(Class)];
            if (!usableCustomElements || info.create.length > 1) {
              notifyAttributes(this);
            }
          }
      }
    });
    safeProperty(proto, ATTRIBUTE_CHANGED_CALLBACK, {
      value: function (name) {
        if (-1 < indexOf.call(attributes, name)) {
          if (CProto[ATTRIBUTE_CHANGED_CALLBACK])
            // -------- FIX ----------------
            // (use `this.__dreInstance` and not `this`)
            CProto[ATTRIBUTE_CHANGED_CALLBACK].apply(this.__dreInstance, arguments);
            // -------- END FIX ----------------
        }
      }
    });

[WebReflection]

I'd like to see a basic example of what is failing and/or where, if possible, so that I can counter-validate any suggested fix, thanks.

P.S. I have the feeling this would be sufficient

  safeProperty(proto, CREATED_CALLBACK, {
      value: function () {
        if (justCreated) justCreated = false;
        else if (!this[DRECEV1]) {
          this[DRECEV1] = true;
          var self = new Class(this);
          if (CProto[CREATED_CALLBACK])
            CProto[CREATED_CALLBACK].call(self);
          var info = constructors[nodeNames.get(Class)];
          if (!usableCustomElements || info.create.length > 1) {
            notifyAttributes(self);
          }
        }
    }
  });

[kamituel]

@WebReflection No, this won't do (just tested).

To be concrete, my issue is when using document-register-element to unit test an application with several custom elements implemented using LitElement. Those are unit tests executed in JSDOM, with mutationobserver-shim and document-register-element (FWIW, runner is Mocha).

LitElement, in reaction to attributeChangedCallback(), invokes the following code:

                if (!this._changedProperties.has(name)) {
                    this._changedProperties.set(name, oldValue);
                }

As it is now, this._changedProperties is undefined when using document-register-element. I can't change LitElement's code easily (and it's correct as far as spec goes, I think). So I probably either need to fix document-register-element - if you agree - or potentially run my tests in a headless browser to avoid having to mock custom elements altogether.

Is it a clear enough explanation? I could potentially provide a repro repo, but not sure if it'll be worth the effort since you grok the issue I think?

[WebReflection]

if you use HyperHTMLElement instead you won't have any issue, as it's been used in production already for years.

I also wonder how come you are using this poly, since "lit people" suggest their own stuff, which I believe has been tested already with their own products.

They've also never been friendly with my OSS, so I'm not putting this in my priority list, but I'll try to figure out in which scenario this could fail.

If you have a basic example code, that would help, thanks.

[kamituel]

if you use HyperHTMLElement instead you won't have any issue, as it's been used in production already for years.

Yup. And maybe I would've had I had chance to start from scratch :)

I also wonder how come you are using this poly, since "lit people" suggest their own stuff, which I believe has been tested already with their own products.

To be quite honest, I didn't notice @webcomponents/custom-elements before :embarassed: It seems to have its own set of issues, from my quick test with it just now (e.g. relying on global object which proves to be problematic with JSDOM, apparently fixed in this 3rd party repo).

At the end of the day though, standard is a standard, and so ideally I should be able to pick your polyfill and use it with another library.

They've also never been friendly with my OSS, so I'm not putting this in my priority list, but I'll try to figure out in which scenario this could fail.

I guess I understand. However, the issue here isn't with LitElement, but rather with document-register-element not following the spec closely, right?

If you have a basic example code, that would help, thanks.

Sure. This is a Mocha test suite:

class Test extends HTMLElement {
  static get observedAttributes () {
    return ['attr']
  }

  constructor () {
    super()
    this._someTestVar = new Map()
  }

  attributeChangedCallback () {
    // Breaks, because this._someTestVar is undefined.
    this._someTestVar.has('dolor sit amet')
  }
}

customElements.define('reproduce-test', Test)

describe('Reproduce', () => {
  it('reproduce', () => {
    let el = document.createElement('reproduce-test')
    el.setAttribute('attr', 'lorem ipsum')
  })
})

With a proposed fix, it works and no exception is thrown. With regular (unfixed) document-register-element it throws:

Error: Uncaught [TypeError: Cannot read property 'has' of undefined]
    at reportException (/home/kamituel/test-app/node_modules/jsdom/lib/jsdom/living/helpers/runtime-script-errors.js:62:24)
    at notifyMutationObservers (/home/kamituel/test-app/node_modules/jsdom/lib/jsdom/living/helpers/mutation-observers.js:166:11)
    at /home/kamituel/test-app/node_modules/jsdom/lib/jsdom/living/helpers/mutation-observers.js:133:5
    at processTicksAndRejections (internal/process/task_queues.js:89:5) TypeError: Cannot read property 'has' of undefined
    at HTMLElement.attributeChangedCallback (/home/kamituel/test-app/dist/webpack:/tests/unit/notifications.spec.js:29:1)
    at HTMLElement.value (/home/kamituel/test-app/node_modules/document-register-element/build/document-register-element.node.js:1363:48)
    at MutationObserverImpl._callback (/home/kamituel/test-app/node_modules/document-register-element/build/document-register-element.node.js:957:55)
    at notifyMutationObservers (/home/kamituel/test-app/node_modules/jsdom/lib/jsdom/living/helpers/mutation-observers.js:158:14)
    at /home/kamituel/test-app/node_modules/jsdom/lib/jsdom/living/helpers/mutation-observers.js:133:5

[kamituel]

I forgot to add that if you're willing to accept a PR with a fix, I can work on one. It doesn't seem very complex, at least from the look I had at this so far, so might be a good "first issue" ;)

[WebReflection]

there you go:

class Test extends HTMLElement {
  static get observedAttributes () {
    return ['attr']
  }

  constructor () {
    // NOPE, this is the part written in the readme
    super()
    this._someTestVar = new Map()
  }

  attributeChangedCallback () {
    // Breaks, because this._someTestVar is undefined.
    this._someTestVar.has('dolor sit amet')
  }
}

So this is what you need to do:

  constructor (...args) {
    const self = super(...args);
    self._someTestVar = new Map();
    return self;
  }

Please let me know if that works, as there's no way to make it work otherwise, thanks.

[WebReflection]

P.S. no, your fix won't work neither, because you'll lose the right context which you need to return from calling super, or next method will fail too.

[WebReflection]

for reference: https://github.com/WebReflection/document-register-element#upgrading-the-constructor-context

closing this, as it's a known, documented, caveat. Happy to re-open if there is a compelling example that fails, even following the must do with the constructor.

[WebReflection]

P.S.2 while this is needed for this poly, calling super() blindly is a foot-gun, because any super can overwrite the returned instance, so if you call super() without addressing it, you might end up dealing with the wrong this, if you don't consider possible upgrades/changes from the called super.

I hope this explain the current state and also avoid foot-gun in the future, with this or any other library where you don't really know what you're extending.

[kamituel]

@WebReflection Unfortunately, I cannot change that class freely. As I explained earlier the class in question is defined in LitElement, and our custom elements inherit from it.

Apologies for the confusion, my code example wasn't matching reality 1:1 as I assumed I gave enough context in earlier comments. This example is more life-like:

class SomeLitClassICannotControl extends HTMLElement {
  constructor () {
    super()
    this._someTestVar = new Map()
  }

  attributeChangedCallback () {
    this._someTestVar.has('dolor sit amet')
  }
}

class Test extends SomeLitClassICannotControl {
  constructor (...args) {
    const self = super(...args)
    return self
  }

  static get observedAttributes () {
    return ['attr']
  }
}

customElements.define('reproduce-test', Test)

describe('Reproduce', () => {
  it('reproduce', () => {
    let el = document.createElement('reproduce-test')
    el.setAttribute('attr', 'lorem ipsum')
  })
})

AFAICT, neither workaround described in the V1 Caveat in the README.md can help in this scenario.

[WebReflection]

once again, the moment you have this:

  constructor () {
    super()
    this._someTestVar = new Map()
  }

is the moment this poly cannot do anything, because even your solution would fail, as you'll have the wrong this context everywhere else, if the super does not return the correct instance.

I'm afraid you need to use their polyfill, as this one upgrades real nodes and you need to return those, 'cause setPrototypeOf is not an option (this polyfill works down to IE9 or even 8, plus every mobile browser to date, while their polyfill works down to IE11 only, and less mobile browsers, so they have easier life with their constructor).

[WebReflection]

To clarify, as soon as any class doesn't follow the caveats, your code is doomed.

I'm sorry you chose lit-element in the first place, but you can still convince them to be compatible with other polyfills that need that (actually right) pattern on their constructor to make things work.

Good luck.

P.S. forks are always an option

[WebReflection]

please note

the caveat is also needed if you use the wrong transpiler, as Babel 7, or native classes, should be able to give you out of the box what you need.

However, the super() needs to be super(...args) because the polyfill passes along the original element.

If that doesn't happen, it cannot work.

[WebReflection]

Also, if I can better explain why that intermediate class is an issue, please see this code:

// the Caveat class mimic an HTMLElemnt (or others) overwrite
// it expects an object (element) and returns it after upgrading it
class Caveat {
  constructor(value = {}) {
    value.caveat = true;
    return value;
  }
}

// now you have lit-element
class SomeLitClassICannotControl extends Caveat {
  constructor() {
    // if you don't pass arguments, you miss the point
    super();
    // no matter what you do after
    this.what = 'ever';
  }
}

// this cannot fix/patch upstream issues
class Test extends SomeLitClassICannotControl {
  constructor (...args) {
    // the argument is ignored in the super constructor
    // so no matter what you pass here, it won't work
    const self = super(...args);
    // this won't be what you expect
    return self;
  }
}

const test = new Test([1, 2, 3]);
console.log(test.caveat);
// true if it's native env
// or if it's Babel 7

console.log(test instanceof Test);
// FALSE !!! Do you see the issue here?
// nothing you created is now usable

console.log(Array.isArray(test));
// FALSE again!

console.log(test.what);
// "ever"

I hope now it's super clear why there's no hope against an intermediate class that doesn't pass along arguments.

If only lit-element would at least use super(...args), everything would work without issues.

[kamituel]

Thanks for the explanation @WebReflection!

Fair enough too. I think I'll use LitElement's polyfill - seems like a simplest thing to do in my case.

Cheers!