aurelia / templating

An extensible HTML templating engine supporting databinding, custom elements, attached behaviors and more.
MIT License
116 stars 104 forks source link

Fix: ensure child observation work properly with native & emulation for content projection #685

Closed bigopon closed 4 years ago

bigopon commented 4 years ago

At the moment, in unbind() of ChildObserverBinder, it's always unset the value via

this.viewModel[this.property] = null;

This works for native shadow DOM, because it's always ensured to retrieve the value again during bind(), as it's safe to do so. Originally, we didn't have this, and it was my attempt of trying to do proper cleanup, and it resulted in this subtle bug between native & emulation. To actually fix it, the above needs to be nested inside a native shadow DOM usage check:

if (this.useShadowDOM) {
  this.viewModel[this.property] = null;
}

For emulation, the content projection is done via an augmented view slot, and thus, can only work when content is actually projected through it, hence .bind()/.unbind() won't, and shouldn't affect it.

This PR also:

@EisenbergEffect

Close #675

EisenbergEffect commented 4 years ago

Looks like you are still working on this so just let me know when you feel that it's ready to merge. @bigopon I'm also fine with you doing the merge and taking on the lead role around au1 templating. You clearly know more than anyone else now. It's been a while since I looked into the depths of this area of code myself, so I trust you more than I trust myself 😄

bigopon commented 4 years ago

@EisenbergEffect All done 2 commits ago. Just this morning I realize the way linting process bails at the moment could easily lead to 2 - 3 follow up commits so I changed it to aggregating all linting errors before exiting. Ready to go any time. With regards to taking the lead, sure I'll take care of that. Though it's good to have someone looking at this fresh.

Mostly it's the change from

  unbind() {
    if (this.viewHost.__childObserver__) {
      this.viewHost.__childObserver__.disconnect();
      this.viewHost.__childObserver__ = null;
      this.viewModel[this.property] = null;
    }
  }

to:

  unbind() {
    if (has observer) {
      // 1. remove this binder
      // 2. check if there's no binder on the observer, and remove the observer itself if 0 binder
      // 3. check if it's safe to unassign the value in the view model, via useShadowDOM property
    }
  }