patch for #591

[dam0vm3nt]

Hey @jakemac53 I send you a possible hack for bug #591.

I found that when polymer calls _propertyChanged and notifyPath it will pass a different path from the original one. Don't know if this is wanted from polymer guys or not. But you will find that array indexes are replaced with Poylmer.Collection keys.

Keys and indexes are aligned until you try to add/remove an element in/from the middle of an array. When keys and indexes are no more aligned the original path and the one passed to _propertChanged are not the same anymore and this will cause troubles... (see #591 for example).

I've added something that will resolve paths taking that into account.

[jakemac53]

Thanks for looking into this! Can you add a test (probably called List.index here) which demonstrates the bug and makes sure it doesn't regress?

[dam0vm3nt]

You are welcome! I can add the test but I fear I will have no time to do it until Wednesday.

Il giorno dom 27 set 2015 19:16 Jacob MacDonald notifications@github.com ha scritto:

Thanks for looking into this! Can you add a test (probably called List.index here https://github.com/dart-lang/polymer-dart/blob/387cf8e65ae2de3b33fbda20bdb64c211f8e9df6/test/src/common/polymer_mixin_test.dart#L41) which demonstrates the bug and makes sure it doesn't regress?

— Reply to this email directly or view it on GitHub https://github.com/dart-lang/polymer-dart/pull/594#issuecomment-143577988 .

[dam0vm3nt]

Whatever, I found some spare time and added the tests. If you run the test on rc.1 without this patch it will fail with an exception. With this patch everything runs fine.

BUT: there is still one big problem. To write the right work-around one should be able to convert back keys into array indexes again. But I've not found a way. I really don't understand what the meaning of Polymer.Collection should be neither why polymer guys are passing a new path based on keys instead of array indexes (see here ). To me there is something wrong on this. Otherwise the only thing I can think of is patching polymer_interop to pass the original path as an extra argument down the chain until it can reach the hooks you made.

[jakemac53]

Ya something really weird is going on here, I am going to continue looking into it today as its a blocker for releasing anything clearly ;)

[jakemac53]

https://github.com/Polymer/polymer/issues/2490

[dam0vm3nt]

OK. Back to normality after a journey in weirdnessland ...

Il giorno lun 28 set 2015 19:57 Jacob MacDonald notifications@github.com ha scritto:

Polymer/polymer#2490 https://github.com/Polymer/polymer/issues/2490

— Reply to this email directly or view it on GitHub https://github.com/dart-lang/polymer-dart/pull/594#issuecomment-143825719 .

[dam0vm3nt]

I've sent you https://github.com/dart-lang/polymer-dart/pull/595 with only the two new testcases. So you're blocked until they will fix it or are you going to patch polymer_interop ?

[dam0vm3nt]

fwiw : changing this line with:

       this.notifyPath(path, value);

will make all the test run fine...

[jakemac53]

Ya it will break other things though internally in polymer. notifyPath only supports paths with keys.

[dam0vm3nt]

I feared that. Otherwise there was no point in doing all that work to replace the path. So I wonder how they will fix that expecially after you noted that notifyPath is a public api. Guess we're just going to wait and see...

[jakemac53]

I am going to work on it with them. For notifyPath this is less bad because the dart side has already been updated, we just need to call it with the correct key instead of the index. The tricky part is responding to updates to an array coming from a JS Element, since we can't map the keys back to indexes. This also only applies to paths which end in a key, so its actually a relatively restricted issue.

[dam0vm3nt]

Well, good to know. As soon as it is fixed I will continue with the work on autonotify. Tnx.