unbind not invoked for custom-elements under root level repeat.for within slot content

[AdamWillden]

I'm submitting a bug report

• Library Version:
  1.4.1

Please tell us about your environment:

• Operating System:
  Windows 10

• Node Version:
  7.50

• NPM Version:
  4.2.0

• JSPM OR Webpack AND Version
  JSPM [email protected]

• Browser:
  Chrome

• Language:
  TypeScript 2.2

Current behavior:
This is a very obscure bug, and is best explained with an example. I'm not sure if the repeat.for is part of the root issue here or not.

  <collapsible-island>
    <h3 slot="header">${dmaFunction}</h3>

    <div repeat.for="loggerLink of loggerLinks">
      <dma-mini-status if.bind="isImport" dma-id.bind="loggerLink.linkedDmaId"></dma-mini-status>
      <dma-status-logger logger-link.bind="loggerLink"></dma-status-logger>
      <dma-mini-status if.bind="isExport" dma-id.bind="loggerLink.linkedDmaId"></dma-mini-status>
    </div>
  </collapsible-island>

Our custom elements <dma-mini-status> and <dma-status-logger> components have bind called on render but unbind is never called.

However if we wrap the default content in a <div> (as shown below) then everything works as normal.

  <collapsible-island>
    <h3 slot="header">${dmaFunction}</h3>

    <div>
      <div repeat.for="loggerLink of loggerLinks">
        <dma-mini-status if.bind="isImport" dma-id.bind="loggerLink.linkedDmaId"></dma-mini-status>
        <dma-status-logger logger-link.bind="loggerLink"></dma-status-logger>
        <dma-mini-status if.bind="isExport" dma-id.bind="loggerLink.linkedDmaId"></dma-mini-status>
      </div>
    </div>
  </collapsible-island>

I'm a bit rushed now so a reproduction is difficult for me to produce right now but will try to follow up soon.

It appears @RWOverdijk and @t03apt are actually experiencing the same issue: https://gitter.im/SpoonX/Dev?at=58e630e468bee3091f17068f

[StrahilKazlachev]

Does this represent your case?

[AdamWillden]

@StrahilKazlachev yes, precisely. If you remove the wrapper as indicated then unbind will not be not called. Thank you very much for putting together that gist. It is very much appreciated!

[bigopon]

@EisenbergEffect Fixed by #592

[jmezach]

It seems that #592 does indeed fix the issue. Unfortunately we've identified another issue in which unbind is not invoked when combined with routing. I've created a gist.run to show this.

In this scenario the subitem and slotitem class's unbind functions are not invoked when navigating, while the other component's unbind functions are called. This can lead to memory leaks since the bindings are kept alive unnecessarily.

[bigopon]

@jmezach Thanks for the gist, it's interesting.

For a work around, wrapping content of <slot-component/> inside a <div/> will help avoid the issue.

[jmezach]

I think the problem occurs when you have a component that has a slot whose content is a component that has a slot itself and a repeat.for is used on that component. I've created another gist.run that shows this behaviour. When you click the replace button you'll see (in the console) that unbind() isn't called on the slot-item component. Adding the dummy div around the in the page.html as suggested above does indeed fix the problem, but it's still a bug IMHO since adding the div might cause layout issues.

In debugging this I came across this _projectionRemoveAll function and in our case the if on that line evaluates to false, thus returnToCache isn't being called which in turn causes the unbind() function not being called on the component. The question is why the isAttached is false here.

[bigopon]

@jmezach isAttached should be false if its parent ViewSlot is already detached. I think the fix introduced didn't cover this case.

[Alexander-Taran]

Maybe can be closed.
Pull request merged