[2.15+] multi brace and arrayproxy break dep/watch chain

[jlami]

Hi,

I have a problem with the following twiddle in ember 2.15+ https://ember-twiddle.com/775f213a59b75008bcccc5056b60069b?openFiles=controllers.application.js%2C

You can also clone my repository here: https://github.com/jlami/ember-bug-multi-brace

I have traced the problem to an issue with the _deps and _watching meta information. The multi brace seems to invalidate the cache and clear these chains, leaving them in a state where the chain is broken and not rebuild. The first propertyDidChange @each will trigger a reread of the whole chain, which rehooks the chain/deps. But the next @each seems to only clear the meta info, and not trigger the whole chain, as the other properties are already seen. Pre 2.15 the watching+deps get reset in a rerender later on. But this does not seem to happen in 2.15 en 2.16 (and also 2.17.beta-5). I don't know how this is triggered. Maybe a dirty flag? Maybe somebody knows what was changed to the dependency chain handling?

BTW debugging this was a bit hard, since 2.16.2 seems to have incorrect source-maps for metal-lib, is this a known bug?

[rwjblue]

2.16.2 seems to have incorrect source-maps for metal-lib, is this a known bug?

I don't recall an open issue for this, maybe you can search and if not report as a new issue?

[rwjblue]

I don't understand the twiddle, every time I check the checkbox it is updating the length properties. How can I reproduce the issue you are reporting?

[rwjblue]

Updated twiddle that seems to not update when it should (haven't dug into the code of the repro to confirm it is or isn't valid) using canary builds: https://ember-twiddle.com/c212fc6f3364e67cb178b51bb7fa1537?openFiles=controllers.application.js%2C

[karthiicksiva]

Might be related to #15843

[jlami]

Sorry for the confusion about the twiddle. Didn't know that you could specify canary or release.

@karthiick I don't know if they are related. That one seems to be a regression from 2.15.3 but this bug seems to already exist there. 2.14.1 seems to have the correct behavior and 2.15.0 breaks it. I'll see if I can find any changes that stand out to me in 2.15.0.

[Serabe]

Playing with the twiddle, showing the correct behaviour "fixes" the issue.

[jlami]

Yes, maybe the correct behaviour is due to a global 'rerender' that is not triggered with the bugged version. But I don't know how the glimmer rerender logic works.

[jlami]

I think this is due to a change in arrayContentDidChange. https://github.com/emberjs/ember.js/blob/v2.14.0/packages/ember-runtime/lib/mixins/array.js#L145 and further only use 'length' under certain conditions. While https://github.com/emberjs/ember.js/blob/v2.15.0/packages/ember-runtime/lib/mixins/array.js#L145 always gets it.

This means that the length property now is calculated way earlier in the update and somehow is not marked as being watched. In my tests with ember 2.14 the length was computed in the glimmer rerender. While 2.16 does it in the chain/deps/watching fase. Somehow the latter makes it that the rerender thinks the value is not changed. But I must say that I'm not really familiar with the glimmer code that tests this. Maybe someone else can take it from here?

[jlami]

I think this has to do with DID_SEEN from dependentKeysDidChange. It might be too eager to say that a dep has already been done. In this scenario the multiEach key actually gets multiple calls to propertyDidChange which should then also invalidate the watching nodes that depend on it. But DID_SEEN remembers that the parent.multiEach has already been done before. This prevents the chains from re-registering their deps and breaks the chain.

This did not manifest itself before, since the chains would register only on the next render because the length was dirty and not recomputed yet. So only one recompute was just fine here. But because the length is now recomputed before the render at the first time parent.multiEach is updated, the chains are in an incomplete state when multiEach has removed its dependencies, but is never recomputed itself which would reset the dep/watch tree.

I could fix the issue by ignoring DID_SEEN altogether by doing this:

function dependentKeysDidChange(obj, depKey, meta$$1) {
    if (meta$$1.isSourceDestroying() || !meta$$1.hasDeps(depKey)) {
      return;
    }
    var seen = DID_SEEN;
    var top = !seen;

    if (top) {
      seen = DID_SEEN = {};
    }
    //HACK TO DISABLE DID_SEEN
    seen = {};

    iterDeps(propertyDidChange, obj, depKey, seen, meta$$1);

    if (top) {
      DID_SEEN = null;
    }
  }

Maybe the changes should be grouped in another way so the array only gets one change? Or should DID_SEEN entries get reset to false if they are told they have to recompute somewhere?

[edit] I don't know what the DID_SEEN is supposed to do, but if it is only for cycle detection we could also just reset current[depKey] = false; at the end of iterDeps

[pixelhandler]

@boris-petrov @jlami @karthiicksiva @rwjblue is this still an issue, perhaps we should close or create a new reproduction of this, what do you think?

[jlami]

Ember 2.18.2 still seems to have this problem in the twiddle, so LTS still has the bug as far as I can tell. But 3.2.2 works in the twiddle. Can't seem to test "release", but I would expect that to work if 3.2 also works. Up to you whether a bugged LTS is worth more investigation.

[locks]

Closing due to 2.18 no longer being an active LTS.