Skip to content
This repository has been archived by the owner on Mar 13, 2018. It is now read-only.

PathObserver doesn't support String keys in index expressions #64

Closed
jolleekin opened this issue Aug 4, 2014 · 8 comments
Closed

PathObserver doesn't support String keys in index expressions #64

jolleekin opened this issue Aug 4, 2014 · 8 comments

Comments

@jolleekin
Copy link

Since observe-dart follows observe-js, I think it is necessary for the team to be aware of the following bug in the Dart implementation.

https://code.google.com/p/dart/issues/detail?id=20294

@jmesserly
Copy link
Contributor

edited subject. Based on your link, I think the issue is property paths like obj["+foo"] don't currently work.

@jmesserly jmesserly self-assigned this Aug 7, 2014
@jmesserly jmesserly changed the title [observe-dart] PropertyPath in observe v0.11.0 still doesn't support Map indexers PropertyPath doesn't support String keys in index expressions Aug 7, 2014
@jmesserly jmesserly changed the title PropertyPath doesn't support String keys in index expressions PathObserver doesn't support String keys in index expressions Aug 7, 2014
@jolleekin
Copy link
Author

No, obj["+foo"] works perfectly. What doesn't work is obj["foo"] and obj[1] if obj is a Map

Why `obj["+foo"] works

obj["+foo"] is correctly parsed into [#obj, "+foo"]

Why obj["foo"] doesn't work

obj["foo"] is parsed into [#obj, #foo] instead of [#obj, "foo"], and #foo may not exist in generated code

Why obj[1] doesn't work if obj is a Map

_getObjectProperty and _setObjectProperty only handle List indexers and ignore Map integer indexers.

_getObjectProperty(object, property) {
  if (object == null) return null;

  if (property is int) {
    if (object is List && property >= 0 && property < object.length) {
      return object[property];
    }
  } else if (property is String) {
    return object[property];
  } else if (property is Symbol) {
  ...
}

bool _setObjectProperty(object, property, value) {
  if (object == null) return false;

  if (property is int) {
    if (object is List && property >= 0 && property < object.length) {
      object[property] = value;
      return true;
    }
  } else if (property is Symbol) {
  ...
}

This problem results from a difference between JS and Dart. In JS, obj.foo and obj["foo"] are identical while, in Dart, they are two completely different things. So not everything that works in JS can work in Dart.

To fix this problem, the parser should reserve indexers (whether they are Map indexers or List indexers) instead of trying to convert them into symbols.

  • identifiers => symbols
  • string indexers => strings
  • int indexers => int's

@jmesserly
Copy link
Contributor

@jolleekin I think you're talking about Dart-specific issues. Those are tracked at https://code.google.com/p/dart/issues/detail?id=20294

@jmesserly
Copy link
Contributor

(This issue tracker is only for issues related to the JavaScript implementation)

@jolleekin
Copy link
Author

I know. The problem is observe-dart is implemented based on observe-js. If something doesn't work in the Dart implementation, it must be reflected back to the JS implementation. That's why I opened this issue.

Sent from my Windows Phone


From: John Messerlymailto:notifications@github.com
Sent: ‎8/‎12/‎2014 1:46 AM
To: Polymer/observe-jsmailto:observe-js@noreply.github.com
Cc: Man Hoangmailto:jolleekin@outlook.com
Subject: Re: [observe-js] PathObserver doesn't support String keys in index expressions (#64)

(This issue tracker is only for issues related to the JavaScript implementation)


Reply to this email directly or view it on GitHub:
#64 (comment)

@jmesserly
Copy link
Contributor

Ah, gotcha. But I wouldn't say it so strongly. There can be differences, if it makes sense. For example, in the case of Map, it's a Dart type and doesn't need (and couldn't) be handled in JavaScript. Same thing with Symbol vs String. That should be addressed in the Dart code (https://code.google.com/p/dart/issues/detail?id=20294). That's why for this bug, we focus only on problems that can be recreated in JavaScript.

@jolleekin
Copy link
Author

Maybe I took sigmund's words too serious ("that is also how the JS implementation is written. We have to follow up with them to make sure we are consistent."). Anyway, if the Dart team agrees there can be differences, this issue can be closed.

@rafaelw
Copy link
Contributor

rafaelw commented Aug 12, 2014

Ok. I'm closing this. If someone feels there is an actual bug in the JS impl, please post a jsbin with expected/actual behavior.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants