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

Fix issue with conditional expressions #41

Merged
merged 1 commit into from
Jul 22, 2014
Merged

Conversation

arv
Copy link
Contributor

@arv arv commented Jul 22, 2014

For conditionals we failed to setup the observer for both branches
as required. This lead to us only observing either the true or false
branch but not both.

Fixes #40

@arv
Copy link
Contributor Author

arv commented Jul 22, 2014

@rafaelw FYI

@jmesserly
Copy link
Contributor

LGTM. Another approach might be to only observe the live branch (either true or false) then if the value of "test" changes, observe the other branch. Seems a bit more complex to code that up though, so probably not worth it. Not sure if there are semantic reasons to avoid evaluating the non-live branch. I wondered about && and ||, but it looks like they are treated as generic binary operators without short circuiting: https://github.com/Polymer/polymer-expressions/blob/master/src/polymer-expressions.js#L290

@rafaelw
Copy link
Contributor

rafaelw commented Jul 22, 2014

Good catch, but I don't think this quite right.

In particular, if whole expression is dynamic (deps) and either branch contains the dynamic sub-expression, this will fails to correctly re-capture deps.

It seems to me the most correct semantics would be:

  1. That expressions with conditionals are marked as dynamic -- and they must re-collect deps when the test flips between truthy & falsy.

  2. Only the branch which is currently active ever gets eval'd. This will avoid surprising errors.

I think an acceptable near-term fix would be to eval test, & both branches IFF |observer| is defined (which means that deps are being collected). Obviously, this fails (2), but we can open a separate bug for that.

@arv
Copy link
Contributor Author

arv commented Jul 22, 2014

@rafaelw Do you want me to make || and && short circuit as well?

I think the best approach for now is probably to always evaluate all branches. We can look into making ?:, || and && only evaluate the needed branch in another PR.

@arv
Copy link
Contributor Author

arv commented Jul 22, 2014

I marked them as dynamic and made them all lazy. PTAL

@rafaelw
Copy link
Contributor

rafaelw commented Jul 22, 2014

lgtm

For conditionals we failed to setup the observer for both branches
as required. This lead to us only observing either the true or false
branch but not both.

Now we mark conditional (as well as || and &&) as dynamicDeps so the
dependencies gets reevaluated as needed.

Fixes googlearchive#40
arv added a commit that referenced this pull request Jul 22, 2014
Fix issue with conditional expressions
@arv arv merged commit 5295321 into googlearchive:master Jul 22, 2014
@arv arv deleted the conditional branch July 22, 2014 21:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some variables in ternary operator are not updating display.
3 participants