Only the first instance of a dom-repeat template gets notified with linkPaths

[trevordixon]

Live Demo

https://jsbin.com/laqilozaci/4/edit?html,output

Steps to Reproduce

• Use linkPaths to link subproperties of multiple array elements to an object.

  this.thing = thing;
  this.things = [{thing}, {thing}, {thing}];
  this.linkPaths('things.0.thing', 'thing');
  this.linkPaths('things.1.thing', 'thing');
  this.linkPaths('things.2.thing', 'thing');

• Use dom-repeat with the array.

  <template is="dom-repeat" items="[[things]]">
  [[item.prop]]
  </template>

• Change a property of the object.

  this.set('thing.prop', !thing.prop);

Expected Results

Each of the dom-repeat instances should be updated according to the new value.

Actual Results

Only the first instance is updated. Without dom-repeat, it works properly:

// OK
[[things.0.thing.prop]]
[[things.1.thing.prop]]
[[things.2.thing.prop]]

Browsers Affected

Tested in Chrome.

Versions

• Polymer: 3.3.0
• webcomponents: 2.2.10

[christielizabeth]

Very helpful. Thanks.

[arthurevans]

That definitely seems like a bug.

Stranger still, if you call linkPaths with the arguments reversed, you get different behavior (still wrong, but different).

[trevordixon]

You mean like this? I expect this not to work:

this.linkPaths('thing', 'things.0.thing');
this.linkPaths('thing', 'things.1.thing');
this.linkPaths('thing', 'things.2.thing');

Because I believe the implementation does something like this:

this._linkedPaths['thing'] = 'things.0.thing';
this._linkedPaths['thing'] = 'things.1.thing';
this._linkedPaths['thing'] = 'things.2.thing';

[trevordixon]

Also, notice that this bug isn't present in Polymer 1.

[arthurevans]

@trevordixon that is what I meant. It looks to me from the code in computeLinkedPaths that the linkage should work either way:

https://github.com/Polymer/polymer/blob/master/lib/mixins/property-effects.js#L453

... but that doesn't seem to be the case.

[trevordixon]

Yes, notifications go both ways once linked. But order matters when linking. https://github.com/Polymer/polymer/blob/master/lib/mixins/property-effects.js#L1759

[kevinpschaaf]

Thanks for the patience. I confirmed the bug and root caused it to the effect de-duping introduced in Polymer 2, which is a detail of how we introduced batching. Since this construction results in a batch of paths through the same object (things) being queued, when we go to flush the batch of pending changes only the binding effect for the first property runs, and the other two get deduped. I believe computeLinkedPaths will need to flush after each property (meaning you can't get batching benefits from linked paths, but that's probably fine since paths can't be batched at the entry point: we don't accept paths in batched setProperties).

[arthurevans]

@trevordixon you're right, I totally missed that.

@kevinpschaaf this detail of linkPaths isn't documented anywhere that I can see--certainly not in the API docs, which list target path and source path args, but don't describe what that means. Maybe we should add text to the API doc? Something like:

Path notifications are forwarded both directions (source to target and target to source).

The target path must be unique. If you need to link a path more than once, specify that path as the source. For example:

linkPaths(target1, source);
linkPaths(target2, source);

[kevinpschaaf]

Yeah, agree on adding docs for noting that the target must be unique (this is to make unlinking O(1)), but that notifications go both directions.

[trevordixon]

I keep running into issues that I think are caused by this bug. I add a call to notifyPath and my issue goes away, but I'm not sure this bug is the root cause, and it makes things much slower in some cases. Is there way a to turn off batching to see if that fixes my problem, lending evidence that it's another manifestation of this bug?

[trevordixon]

This keeps causing me problems. I'll probably have to refactor a couple of components that rely on two-way binding and linkPaths to instead just use one-way binding, unless there are plans to address this bug. @kevinpschaaf How hairy is the code? Could somebody with little knowledge of Polymer internals fix this bug?

[kevinpschaaf]

@trevordixon It's unfortunately probably the hairiest part of the Polymer codebase. I had looked into this a little after it was filed, and finding a solution seemed seemed pretty over-constrained (i.e., a fix would risk changing an invariant that would break other things). Eliminating the two-way bindings is likely just going to be better overall for your codebase's health, so if that's possible, I'd recommend that.

But, in thinking about the problem a little more this morning, I think I may have made some progress on a possible solution, so wanted to note that here so it's not lost:

The crux of the issue can be reduced to a question of what happens under these conditions:

observers: ['stuffChanged(stuff.*)']

el.setProperties({
  'stuff.x': 'x'
  'stuff.y': 'y'
});

(Note, the API docs note that batching paths in setProperties is not allowed, but as it turns out, linkPaths effectively causes a batch of paths to be set, so we can think about the problem in the same way.)

The question is: How should stuffChanged be called? Currently, because all property effects like observers, bindings, etc. are only run once within a batch of changs, it will be called once, with stuffChanged('a', {base: stuff, path: 'stuff.x', value: 'x'}), and will never be called with the second info object for the second path. This is why we see dom-repeat only ever seeing the first change to things.0, and the things.1 and things.2 changes getting dropped.

We can't simply not dedupe, since batching & deduplication is how we were able to remove the ill-fated "undefined rule" from 1.x where we didn't call observers if an argument was undefined. It's the system that ensures that stuffChanged is only called once at startup for this case:

properties: {
  a: { value: 'a' },
  b: { value: 'b' },
},
observers: ['stuffChanged(a, b)']

However, after thinking a bit more about it this morning, it does seem like we could specifically treat paths matching a wildcard specially and not de-duplicate those effects. The rule would be "if a path matches a wildcard argument, we always call (or re-call) the observer for that, to avoid the information about a path change to be lost".

While this would fix the original issue noted here, it would cause a minor change to an observer like this:

observers: ['stuffChanged(a, stuff.*)']

el.setProperties({
  a: 'a',
  'stuff.x': 'x'
  'stuff.y': 'y'
});
// Current (called once, path information lost):
//   stuffChanged('a', {base: stuff, path: 'stuff', value: stuff})
// With change (called for each path):
//   stuffChanged('a', {base: stuff, path: 'stuff', value: stuff})
//   stuffChanged('a', {base: stuff, path: 'stuff.x', value: stuff.x})
//   stuffChanged('a', {base: stuff, path: 'stuff.y', value: stuff.y}) 

The change, of course, is that we'd potentially be calling observers/effects more than we previously did, which could break users. I think that might generally be fine, since the setup with batching above can only technically be caused by linkPaths today (since we never supported calling setProperties with paths), and the behavior is just broken in that scenario right now. We'd have to double-check whether this breaks users in practice or not (these types of changes tend to bring lots of edge cases out of the woodwork), but this could be a possible path to explore if avoiding this behavior is not possible in your codebase.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.