-
Notifications
You must be signed in to change notification settings - Fork 27
Document moment #66
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Document moment #66
Conversation
deklanw
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise looks good 👍
README.md
Outdated
| the behaviour and the value of the occurrence, the returned value | ||
| becomes the next value of the behavior. | ||
|
|
||
| #### `moment<A>(f: (sample: <B>(b: Behavior<B>) => B) => A): Behavior<A>`` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra ` messes up formatting
README.md
Outdated
| ``` | ||
|
|
||
| The above could also be achieved with `lift`. However, `moment` can do many | ||
| things that `lift` can't. For instance, `moment` can give better performance |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why better performance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the question.
If you write something like this:
const lifted = lift((a, b, c, d) => a && b ? c : d, aB, bB, cB, dB);Then the resulting behavior will always depend on both aB, bB, cB, dB. This means that if any of them changes then the value of lifted will be recomputed. But, clearly we can see that if, for instance, aB is false then the function actually only uses aB and there is no need to recompute lifted if any of the other behaviors changes. However, Hareactive can't know that since the function is just a "black box".
If, on the other hand, we use moment:
const momented = moment((at) => at(aB) && at(bB) ? at(cB) : at(dB));Then Hareactive can simply check which behaviors are actually sampled inside the function passed to moment, and it uses this information to figure out which behaviors momented depends upon in any given time. This means that when aB is false then Hareactive can figure out that currently momented only depends on atB and there is no need to recompute it when any of the other behaviors changes.
Does that make sense or should I attempt to explain it better? Should I explain it better in the documentation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That explanation makes sense to me. I think you could put that right into the docs. (Also, the typo above!)
a3c45b4 to
c13b033
Compare
Codecov Report
@@ Coverage Diff @@
## master #66 +/- ##
=======================================
Coverage 95.65% 95.65%
=======================================
Files 13 13
Lines 1359 1359
Branches 60 60
=======================================
Hits 1300 1300
Misses 59 59Continue to review full report at Codecov.
|
1 similar comment
Codecov Report
@@ Coverage Diff @@
## master #66 +/- ##
=======================================
Coverage 95.65% 95.65%
=======================================
Files 13 13
Lines 1359 1359
Branches 60 60
=======================================
Hits 1300 1300
Misses 59 59Continue to review full report at Codecov.
|
|
Thanks a lot for the feedback @deklanw 😄 I've updated the PR as you suggested and fixed the typo you caught. |
Related to #64. What do you think @deklanw? Does this make sense?