Skip to content
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

Not really PR but rather annotations and questions inline #31

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Not really PR but rather annotations and questions inline #31

wants to merge 8 commits into from

Conversation

dmitriz
Copy link
Contributor

@dmitriz dmitriz commented May 9, 2017

This is not meant a PR but rather a result of my understanding attempt
with annotations and inline questions caused by some confusion,
that I would like to share this way.

Especially I'm puzzled how this inline plain object generates a vnode:
https://github.com/dmitriz/turbine/blob/annotate/examples/todo/src/TodoApp.ts#L79

Would be great to hear your feedback on these and other questions/remarks.

Sorry for the generated .js files that unfortunately got committed for some reason,
not sure whether it was intended, please ignore them here.

@codecov
Copy link

codecov bot commented May 9, 2017

Codecov Report

Merging #31 into master will decrease coverage by 0.2%.
The diff coverage is 87.5%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #31      +/-   ##
==========================================
- Coverage   94.14%   93.93%   -0.21%     
==========================================
  Files           5        5              
  Lines         393      396       +3     
  Branches       62       62              
==========================================
+ Hits          370      372       +2     
- Misses         23       24       +1
Impacted Files Coverage Δ
src/dom-builder.ts 98.38% <100%> (+0.02%) ⬆️
src/elements.ts 85.18% <50%> (-3.28%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5066236...6880b2c. Read the comment docs.

@dmitriz
Copy link
Contributor Author

dmitriz commented May 9, 2017

Now removed the js and yarn stuff

@limemloh
Copy link
Member

limemloh commented May 9, 2017

Thank you for showing interest in Turbine. I will try to explain some of the code with comments on the lines during the next day or two, but for now I will try to clearify this:

Especially I'm puzzled how this inline plain object generates a vnode:
https://github.com/dmitriz/turbine/blob/annotate/examples/todo/src/TodoApp.ts#L79

I see a component in Turbine as a container holding descriptions of:

  1. How to build the DOM.
  2. How the internal logic should be.
  3. What the component should output.

Since components are just descriptions, we can do this:

const btn: Component = button("Click me");

const view: Component = div([
  btn,
  btn,
  btn
]);

Here both div and button are functions returning a component.
When running a component the description is used to build the DOM
The view component will result in:

<div>
  <button>Click me</button>
  <button>Click me</button>
  <button>Click me</button>
</div>

@dmitriz
Copy link
Contributor Author

dmitriz commented May 10, 2017

@limemloh Many thanks for your quick explanation!

I have found my confusion was to having missed the yield statement that did return the node. So perhaps that was the object passed, rather than the one returned.

I must be still confused about how the div function works exactly.
Here is the simpler example:

div(
  function* () {
    const
      select1 = yield selectorButton("1", selected),
      select2 = yield selectorButton("2", selected),
      select3 = yield selectorButton("3", selected),
      select4 = yield selectorButton("4", selected);
    return { selectVersion: combine(select1, select2, select3, select4) };
  }
)

So the div function is passed the four button components as the generator yield statements, and then again in the return object, combined inside the selectVersion property.

Playing with it in console, I got the feeling that the yields are responsible for the initial vnode, whereas the return part is for the future activity.

In the code, this Component is passed to the modelView function, with two plain functions passed as model and view:

const versionSelector = modelView<FromModel, FromView>(

  function ({ selectVersion }) {
    const selected = stepper("1", selectVersion);
    return Now.of([{ selected }, { selected }] as [FromModel, FromModel]);
  },

  function ({ selected }): Component {
    return div(
      { class: "btn-group" }, 
      function* () {
        const
          select1 = yield selectorButton("1", selected),
          select2 = yield selectorButton("2", selected),
          select3 = yield selectorButton("3", selected),
          select4 = yield selectorButton("4", selected);
        return { selectVersion: combine(select1, select2, select3, select4) };
      }
    );
...

The second view function seems to use the selected argument property to control its button components, right?

In the counter example, version2, we have

    button({ 
      // produce output {incrementClick: clickStream} ?
      output: { incrementClick: "click" } }, 
      " + "
    ),

I am somewhat confused about the type of the output, does it match the Model's input?

type CounterModelInput = {
  incrementClick: Stream<any>,
  decrementClick: Stream<any>
}

So is it an object with Stream values?
Or an arrays as the following description indicates:

The div function will combine all the objects output by the components in the array passed to it and output that.

From the usability perspective, array output would be painful, because every time you change the order of your elements, you have to adjust the types. Also the order of the elements reflects their order in the dom, which may be different from how you see the output ordered, if going this way.

But I would still find the object output easier to use, and matching the input type of the Model would seem to be the easiest.

Comparing to Redux, the output correspond to the actions, which in the Redux's case you have to mark with the action types, making them noisier. In case of turbine's outputs, combining them into the object right away can nicely solve this problem. That way the streams are also nicely separated, rather than merged like in Redux, so you don't need to react to unwanted events and filter through all foreign actions.

As the syntax matter, I'd love it to make as terse as possible, e.g.

button({ 
    clickStream: 'increments'
})

So that would tell me, the click event stream is sent to output as the object
{increments: myStream}...

However, I don't feel that way it is expressive enough, as it looks like ordinary object,
so too easy to overlook. So perhaps, building on your idea to user generators, something like that

button({ 
    onclick: yield 'increments'
})

would instantly alert the reader that something unusual happens, which is good.

There is some design problem though, namely, what if two buttons try to use the same prop:

button({ 
    onclick: yield 'increments'
})
button({ 
    onclick: yield 'increments'
})

One solution would be to empower this way by merging both streams,
so the parent would output the same object prop with the streams merged.
Which is often the intention in those cases.
Is that how Turbine currently behaves?

So I see the modelView passing the view output to the counterModel.
And if I see it correctly, the model will only execute once, in contrast to the Redux reducers,
which run on every action.
With Redux you need to feed back the state into the reducer
becaus of that.
So that nicely explains why no state stream is passed along with the action.

The output passed to the counterModel also nicely resembles CycleJS, action object passed to what they (somewhat confusingly) call "reducer":

function reducers(actions) {
  const moveHighlightReducer$ = actions.moveHighlight$
    .map(delta => function moveHighlightReducer(state) {
      const suggestions = state.get('suggestions')
      const wrapAround = x => (x + suggestions.length) % suggestions.length
      return state.update('highlighted', highlighted => {
        if (highlighted === null) {
          return wrapAround(Math.min(delta, 0))
        } else {
          return wrapAround(highlighted + delta)
        }
      })
    })
...
  return xs.merge(
    moveHighlightReducer$,
...
  )
}

https://github.com/cyclejs/cyclejs/blob/master/examples/autocomplete-search/src/app.js#L143

Which is perhaps a sign the design is good :)

Now, pushing further the comparison, Cycle lets the reducer output a single stream,
which they feed to the view as the state stream. While I can see the merit, I feel that adds to the complexity. So the action is an object of streams but the state is the single stream! Even if makes sense, it can be hard to watch for correct types and not to mix the two.

I would find it easier to keep the consistency
and make both inputs and outputs always objects of streams.
That feels more powerful for the separation reason I mentioned above.
Now it seems this is exactly the current Turbine's design, which I like :)

But I would like to avoid the counterModel returning an array, because it is painful to remember, which one comes first. In fact, I am not sure this should be the Model's responsibility to decide how to split its outputs.
This also imposes a restriction on the Model's return type,
and may create additional overhead when you only want to return one of the two or even none.

From the user's perspective, it would be simpler not to think about it, and following the same consistency principle, return an object of streams. Then it is up to its view sister and the parent component to each pick the right ones.


Finally, this line is perhaps the trickiest to digest:

  const count = yield sample(
    scan((n, m) => n + m, 0, changes)
  );

It seems to go through the stateful scan
to feed it into the sample, which returns a Now.

I understand that the sample "corrects" the impure nature of the scan, but then maybe the whole combination can be replaced by something pure, so you don't see any impure function inside? Somehow, if looking for purity, we should not need a complex method to "correct" the purity of something impure. Instead, perhaps, some pure method like a "scanNow" can be introduced?

This seems to resonate with what Andre @staltz recently wrote on his blog about Hot vs Cold, and how that caused some difficulty in Cycle, where the impure methods like the scan have to be tolerated.

At least, this is my interpretation, and I feel it is worth to try to better explain things clearly what they are, why the extra complexity is needed, and how it helps to solve the problems. I find the Hareactive does quite a good job explaining the Now, but would also like to see how it fits into the (painful) Hot-Cold-Observable world.

Any corrections, answers to questions posed and more explanation will be greatly appreciated.

@dmitriz
Copy link
Contributor Author

dmitriz commented May 10, 2017

This interesting discussion might be related:
cyclejs/cyclejs#365

@limemloh
Copy link
Member

I must be still confused about how the div function works exactly.

The div function and all the functions in Turbine's element object are pretty much the same.
They take an object with configurations and/or an optional child component. Here the child component is overloaded to be more practical. If you pass an array of components, it will chain them all to one component, if you pass a generator function it uses the fgo from jabz and if you pass a string or number it wraps them into a component.

One solution would be to empower this way by merging both streams,
so the parent would output the same object prop with the streams merged.
Which is often the intention in those cases.
Is that how Turbine currently behaves?

No, at the moment Turbine merges the objects like Object.assign, so with the following code:

const btn: Component = button({output: {clickStream: click}}, "Click me");
const view: Component = div([
  btn, // 1
  btn, // 2 
  btn  // 3
]);

When view is run, it would output clickStream, representing clicks on btn // 3 only. This might not be the best solution, but there is a bit more to it than just merging the streams; since a component's output object can contain behaviors and constants as well.
If we chose to merge those that were of the same name and were streams, It would be practical in some situations, but I think it would also make the code harder to understand, because it would hide the fact that the stream is from multiple sources.

I understand that the sample "corrects" the impure nature of the scan, but then maybe the whole combination can be replaced by something pure, so you don't see any impure function inside? Somehow, if looking for purity, we should not need a complex method to "correct" the purity of something impure. Instead, perhaps, some pure method like a "scanNow" can be introduced?

The implementation of scan in hareactive is pure, but only because it returns a behavior of the scan. Semanticly a behavior is a function from Time to a value, scan returns a function from startingTime to a result of the scan from this startingTime. @paldepind might be better at explaining this.

but to hide away sample would not be a problem

function scanNow(...args) {
  return sample(scan(...args));
}

but scan should still be part of the hareactive API, since it can be useful in combination with other functions than sample

@dmitriz
Copy link
Contributor Author

dmitriz commented May 12, 2017

@limemloh

The div function and all the functions in Turbine's element object are pretty much the same.
They take an object with configurations and/or an optional child component. Here the child component is overloaded to be more practical.

Thank you for the explanation, I think my confusion comes from the Mithril way, where every component must be wrapped with the element creator before included as child. And I like how the Turbine hides this complexity instead, so you can inline the component directly 😄

If you pass an array of components, it will chain them all to one component,

input().chain(
    inputOutput => span(inputOutput.inputValue)
)

I can see it as the easiest implementation to place it simply after the input component,
but I wonder if that does not lose some power that way, when you for instance want to put it before rather than after (or somewhere else):

span(
    // how can I get the input's output here?
),
input()

When view is run, it would output clickStream, representing clicks on btn // 3 only. This might not be the best solution, but there is a bit more to it than just merging the streams; since a component's output object can contain behaviors and constants as well.
If we chose to merge those that were of the same name and were streams, It would be practical in some situations, but I think it would also make the code harder to understand, because it would hide the fact that the stream is from multiple sources.

Then how would you go if you actually do want to merge them?
Provided they can be all made streams, which seems the case in most situations I can think of.

Are there sufficient use cases to justify this complexity, without which the problem wouldn't even exist? :)

The implementation of scan in hareactive is pure, but only because it returns a behavior of the scan. Semanticly a behavior is a function from Time to a value, scan returns a function from startingTime to a result of the scan from this startingTime.

Oh yes, I have missed the double behavior type ;)
It could also be a Behavior of Streams, without throwing away the event part.
I have a bit concern about taking the behavior part only,
which feels like throwing away the information about the events moments,
that you may like to keep sometimes.

Also the name scan is perhaps somewhat dangerous to use here for something so different from other libraries.

@limemloh
Copy link
Member

limemloh commented May 12, 2017

I can see it as the easiest implementation to place it simply after the input component,
but I wonder if that does not lose some power that way when you, for instance, want to put it before rather than after

Yes, the array overload will remove some power, but together with the loop function, you can still achieve the same result.

const view = loop(({inputValue}) => div([
  span(inputValue), 
  input()
]));

When view is run, it would output clickStream, representing clicks on btn // 3 only. This might not be the best solution, but there is a bit more to it than just merging the streams; since a component's output object can contain behaviors and constants as well.
If we chose to merge those that were of the same name and were streams, It would be practical in some situations, but I think it would also make the code harder to understand because it would hide the fact that the stream is from multiple sources.

Then how would you go if you actually do want to merge them?
Provided they can be all made streams, which seems the case in most situations I can think of.

I came up with these ways:

This is the one I would use in most cases, since the buttons probably would have different text

const view = div([
  button({output: {clickS1: click}}, "Click me");
  button({output: {clickS2: click}}, "Click me");
  button({output: {clickS3: click}}, "Click me");
]).map({clickS1, clickS2, clickS3} => ({clickStream: combine(clickS1, clickS2, clickS3)}));

if the buttons actually was suppose to be identical we could do this:

const btn = (name) => button({output: {[name]: click}}, "Click me");
const view = div([
  btn("clickS1"),
  btn("clickS2"),
  btn("clickS3")
]).map({clickS1, clickS2, clickS3} => ({clickStream: combine(clickS1, clickS2, clickS3)}));

But say that I was using a component from som library, and their API did not allow us to specify the name of the output, I would use a generator like this:

import {btn} from "turbine-bootstrap";
const view = div(function*(){
  const {clickStream: clickS1} = yield btn;
  const {clickStream: clickS2} = yield btn; 
  const {clickStream: clickS3} = yield btn;
  return { clickStream: combine(clickS1, clickS2, clickS3)};
});

But if we really wanted to use the array overload, we could do this:

import {btn} from "turbine-bootstrap";
const rBtn = (name) => btn.map(({clickStream}) => {[name]: clickStream});
const view = div([
  rBtn("clickS1"),
  rBtn("clickS2"),
  rBtn("clickS3")
]).map({clickS1, clickS2, clickS3} => ({clickStream: combine(clickS1, clickS2, clickS3)}));

About whether it is mostly streams the components will output, I do not agree. Since Turbine components hold state mostly in behaviors and other components might depend on this state, the output would be a mixture of streams and behaviors.
Only a very few of the DOM element components hold state, so when working only with these, it mostly would be streams.

@limemloh
Copy link
Member

limemloh commented May 12, 2017

It could also be a Behavior of Streams, without throwing away the event part.
I have a bit concern about taking the behavior part only,
which feels like throwing away the information about the events moments,
that you may like to keep sometimes.

yes, indeed. There is another variation of scan called scanS which returns a behavior of a stream.

Also the name scan is perhaps somewhat dangerous to use here for something so different from other libraries.

I think that hareactive's scan behaves pretty much like a scan in other FRP libraries, the big difference is that the API of hareactive makes the user aware that it is at a given point in time that the scan starts from.

@dmitriz
Copy link
Contributor Author

dmitriz commented May 14, 2017

@limemloh

Thank you again for your answers!

const view = loop(({inputValue}) => div([
  span(inputValue), 
  input()
]));

This feel somewhat "magic".
How is it specified here where the intputValue coming from?

I came up with these ways:

Thanks, interesting to see so many ways :)
Having thought about it, I agree that merging by default looks artificial.
And collecting into array would possibly be the most natural one,
as that is exactly the children structure in the dom.

This would be the simplest possible syntax I could think of:

const view = div([
  button({ output: 'click'  }),
  button({ output: 'click'  }),
]).map([clickStream1, clickStream2] => ({clickStream: combine(clickS1, clickS2)}));

Would it make sense?
That way you could reference the outputs directly without introducing the props.

BTW, you seem to rename merge into combine here?
I see it follows the Hareactive and Jabz convention,
but seems to deviate from other stream libraries incl. Flyd.
Will it not create some confusion for people using other libraries?

About whether it is mostly streams the components will output, I do not agree. Since Turbine components hold state mostly in behaviors and other components might depend on this state, the output would be a mixture of streams and behaviors.

Yes, this point was well-made by @paldepind in his blog post :)

yes, indeed. There is another variation of scan called scanS which returns a behavior of a stream.

You are right, I was confused by the word "stateful". Now I am not even sure why it is call that way. Was adding the behavior part not meant to remove the statefulness?

I think that hareactive's scan behaves pretty much like a scan in other FRP libraries, the big difference is that the API of hareactive makes the user aware that it is at a given point in time that the scan starts from.

The biggest difference is the different return type. It might cause some errors if you are not careful I think. Also the usual scan implicitly "knows" the creation time. But here, I am not quite sure to understand how to "remember" this time in a functional way. Even with Now you would only be able to use the current time but not the one when the scan occurred? Am I confusing something?

@paldepind
Copy link
Member

@dmitriz

This feel somewhat "magic". How is it specified here where the intputValue coming from?

inputValue is coming from the function itself. If the function returns Component<A> then it will receive A as input. And yes, it seems magical. The documentation for loop admits that.

But, at a "theoretical level" it isn't magic. What loop gives you is the fixed point of a monadic computation. It's like the fixed-point combinator but for monads.

If I understand correctly you've used Cycle. You know that the framework is based on a single global circular dependency chain. What we've realized is that circular dependencies happen very frequently in FRP. And that only having a single global circular dependency is inconvenient.

Thus we provide loop that makes it possible to create as many dependency cycles as one needs. The modelView function also creates a cyclic dependency in this way. You can see that by looking at this figure:

modelView figure

So in a sense, each component created by modelView has it own "cycle". That is very powerful.

@dmitriz
Copy link
Contributor Author

dmitriz commented May 14, 2017

@paldepind

inputValue is coming from the function itself. If the function returns Component then it will receive A as input. And yes, it seems magical. The documentation for loop admits that.

LOL. I think what I'd like to see is how the inputValue exported, with the rest of the magic being ok, something like that:

const view = loop(({inputValue}) => div([
  span(inputValue), 
  input({ output: { inputValue }})
]));

That is, I like my components to make it clear what they want to output.
Similar to functions, where you keep things private and only export what needed,
I'd like my input only export what I write next to it, and no other events, even if those are present. That is exactly the same problem people incl. myself had with CycleJS, where it is impossible to limit how your component is accessing its dom.

But, at a "theoretical level" it isn't magic. What loop gives you is the fixed point of a monadic computation. It's like the fixed-point combinator but for monads.

It is an interesting point of view. :)

If I understand correctly you've used Cycle. You know that the framework is based on a single global circular dependency chain. What we've realized is that circular dependencies happen very frequently in FRP. And that only having a single global circular dependency is inconvenient.

I have tried to use Cycle and worked through the examples but hit several walls, with this one you mention being exactly one of them. Without even being clearly mentioned there.

Which made me really love your diagram when I saw it in Turbine :)
This is exactly what the "fractal" architecture should be,
which ironically is the term used by the Cycle itself. :)

@dmitriz
Copy link
Contributor Author

dmitriz commented May 15, 2017

Here are some points about the scan impurity, related to the above:
#28 (comment)

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

Successfully merging this pull request may close these issues.

3 participants