expando thing on jsValue for jsArray #16
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
|
I signed it! |
|
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. |
| @@ -21,13 +21,19 @@ abstract class JsProxyInterface { | |||
| JsObject get jsProxy; | |||
| } | |||
|
|
|||
| Expando<JsArray> jsArrayExpando = new Expando<JsArray>(); | |||
|
@sigmundch What do you think about this? The primary risk here is if the two lists could get out of sync there is no way to recover from it, since the JsArray is now permanently tied to the Dart List. |
|
mmm interesting - it's a valid concern. Wouldn't you run into other issues if you get out of sync, even without this? I also have a couple questions:
|
| if (newList==null) { | ||
| newList = new JsArray.from(dartValue.map((item) => jsValue(item))); | ||
| jsArrayExpando[dartValue] = newList; | ||
| } | ||
| addDartInstance(newList, dartValue); |
There was a problem hiding this comment.
may be addDartInstance should be pulled inside if statement.
Yes, otherwise it would be inconsistent. @dam0vm3nt can you add that into this PR?
A bit riskier, but I still think we should. @dam0vm3nt can you do this as well? |
|
done. check it out. |
|
Nice, can you also resolve the CLA issue? I think the problem is that your git commits are using a different email than your github account. |
I've added the email address to my github account, don't know if this is sufficient... |
|
CLAs look good, thanks! |
|
Ok, googlebot just answered :) |
expando thing on jsValue for jsArray
As suggested by @jakemac53 this will fix auto notification support for jsArray.