Combine observables using merge/scan #140
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mariusmuja
force-pushed
the
combine_observables
branch
from
bbc78dd
to
cf95f8a
Compare
fdietze
force-pushed
the
combine_observables
branch
from
cf95f8a
to
1fe5c42
Compare
fdietze
reviewed
Jan 6, 2018
| Observable(Seq.empty) | ||
| } | ||
|
|
||
| // only use last encountered observable per attribute |
There was a problem hiding this comment.
This is very much at the core of the library, all tests that contain VDOM Observables are tests for it.
There was a problem hiding this comment.
I was referring to:
// only use last encountered observable per attribute
I was just wondering if we have a test for this, else I would write one.
There was a problem hiding this comment.
mariusmuja
force-pushed
the
combine_observables
branch
from
1fe5c42
to
91c11ac
Compare
fdietze
force-pushed
the
combine_observables
branch
from
91c11ac
to
61a1e80
Compare
mariusmuja
force-pushed
the
combine_observables
branch
from
61a1e80
to
0eada3d
Compare
|
I'd like to get a second review on this before merging it. |
mariusmuja
force-pushed
the
combine_observables
branch
from
0eada3d
to
e0944d5
Compare
cornerman
reviewed
Jan 23, 2018
| } | ||
| } | ||
|
|
||
| private[outwatch] case class VNodes(nodes: List[ChildVNode], streamStatus: StreamStatus) extends Children { | ||
| private[outwatch] case class VNodes(nodes: List[ChildVNode], hasStream: Boolean = false) extends Children { |
There was a problem hiding this comment.
I would remove the default argument here, because you can easily create an invalid state here, so you should think about this argument when constructung this VNodes class.
cornerman
approved these changes
Jan 23, 2018
|
@mariusmuja Could you kindly rebase on master? :) |
mariusmuja
force-pushed
the
combine_observables
branch
from
e0944d5
to
766238c
Compare
|
Rebased on master. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR refactors the way the VNode observables are combined, using
merge&scan(and explicit VNode state) instead ofcombineLatest.I think it makes the code simpler and removes the need for workarounds such as keeping track if there's one or more child observables or making sure there's a
startWithon the streams for partial rendering, avoiding issues such as #137, #134.