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

on keeping all the reducing functions on the store module aka stow operator #16

Closed
gabrielmontagne opened this issue Feb 15, 2019 · 16 comments · Fixed by #24
Closed

on keeping all the reducing functions on the store module aka stow operator #16

gabrielmontagne opened this issue Feb 15, 2019 · 16 comments · Fixed by #24

Comments

@gabrielmontagne
Copy link
Contributor

So far we've been ambivalent about where we should keep our final "to reducer" maps---whenever we need access to the "unplugged" stream we start exposing streams with ugly names like rawLocation$...

But with the comfort of immutable and sweetness of pipeable operatos---thinking something like

function update(keyOrPath) {
  return function update(source$) {
    return source$.pipe(
      map(value => state =>
        Array.isArray(keyOrPath)
          ? state.setIn(keyOrPath, value)
          : state.set(keyOrPath, value)
      )
    )
  }
}

... one can keep all merged streams on the store's merge( emitting "real" data. This allows them to be always routable. And we then can leave the final "but where on the state should this be plugged?" answers in one place: on the store$ definition:

const store$ = merge(
    piece$.pipe(update('piece')),
    chapter$.pipe(update('chapter')),
    whatever$.pipe(update(['piece', 'wherever']))
).pipe(
  scan((state, changeFn) => changeFn(state), initialState),
  // etc etc.

The name of the operator is not great, and maybe it could be split into dumber versions (update, updateIn), but how does it sound? @cristiano-belloni ?

With these new RxJS+Immutable idioms and the state context, apps could get very very lean, I'm feeling. Hopefully not terribly magical, but only sensibly magical. And context wired router constructs is yet something we haven't considered.

@cristiano-belloni
Copy link
Collaborator

So every stream will be unplugged and (consequently) directly "usable"? I like it.

(This assumes that everything we want to do in a reducer is a set of a value. Maybe there are cases where we want to write more complex reducers, for example reducers that are dependent on the current snapshot of the state itself (with update or updateIn*)? Do those cases ruin our beautiful illusion of a pluggable world of streams?)

*Yes, maybe the name should be changed - it conflicts in weird ways with the Immutable API.

@gabrielmontagne
Copy link
Contributor Author

Cool, yes, we can unpack a small set set setIn update updateIn to keep them aligned with Immutable.

And, yes, I think it becomes clearer that store$ is the name of the final merge of those streams just for rendering. If one feels tempted to use it further up for sure one can plug in the composed streams directly instead, as they wouldn't be plugged with reducer functions anymore.

@cristiano-belloni
Copy link
Collaborator

cristiano-belloni commented Feb 16, 2019

The old "tree of life" way, agreed.

Not sure about

we can unpack a small set set setIn update updateIn to keep them aligned with Immutable

For sure the observable chain maps to a value at the end, so set and setIn are ok, as in your example code. On the other end, we shouldn't implement a way to use update and updateIn, because we would need streams that map to a value and an update function, which is the thing we want to avoid (and handle that kind of thing composing streams above, as you said).

For this reason, I am ok with your original example implementation, just let's not call it update because it name-clashes with what we don't want to support in the Immutable API and it feels weird (ah, ok, it's called "update", so it does an immutablejs update under the hood? No, it does a "set").

Another thing that comes to mind is adding another check to see if the passed argument is an observable, and merge it in that case, but I'm not sure it's really useful and probably will just try to do too much stuff.

@cristiano-belloni
Copy link
Collaborator

Question: should we also support

  • delete and deleteIn
  • add and delete (for Sets)

These are examples which are not cleanly solved with some data-massaging + a simple set.

(One additional edge case would be a ordered-type's insert, which could be emulated by re-calculating the new list and setting the new value at the end)

@gabrielmontagne
Copy link
Contributor Author

Cool. Sounds fun. :-) I think we should play with it a bit before we explode in APIs. But why not, if it turns out that we can cover all cases easily.

@gabrielmontagne
Copy link
Contributor Author

This also forces us to think on some of our utils---createNavigateTo$, createLoader$, etc. that currently work by emitting the reducer straight away. They could work a bit differently, I guess, but connecting to a dedicated stream which is the one we finally plug into the store with that final reducer. This could help us with the kind of problems we had on the portal, trying to have a location$ stream that we could tap from.

Newer version would still come in as steps on a .pipe(, but would have their output come out on different streams? Loops in the mangrove?

@cristiano-belloni
Copy link
Collaborator

That makes sense but I can't imagine it in code. If you have something, could you do a very-early-just-a-wip PR?

@cristiano-belloni
Copy link
Collaborator

cristiano-belloni commented Feb 21, 2019

So we won't have a

 Observable.merge(
  ...
  journeyThatUsesCreateNavigateTo$,
  ...
)

anymore.
Actually, now that I think about it, journeys can emit a lot of different reducers. The proposal is to subdivide them in such a way that anything that merges into the store just emits one single "kind" of value? This means, in other words, not being able to mix loaders with location changes with actual "other" data changes in journeys?

@gabrielmontagne
Copy link
Contributor Author

gabrielmontagne commented Feb 22, 2019

After your last comment, @cristiano-belloni , I'm having second thoughts about this approach. I guess one could be pragmatic---plug in simple streams into the store, but emit reducers in complex journeys. Can you imagine CC if we got all fundamentalist about this? It wouldn't help much, I think. But in some places it might.

This is all to say: good point! I'm not sure about that.

@cristiano-belloni
Copy link
Collaborator

I guess that the good old solution of having reducer-emitting streams and split them if we need a more generic functionality wins in terms of flexibility.

@gabrielmontagne
Copy link
Contributor Author

gabrielmontagne commented Feb 22, 2019 via email

@cristiano-belloni
Copy link
Collaborator

So, in practice, we could have the update utility (or whatever we're gonna call it) in case we need a fast reducer for a simple, single-valued stream, but not "mandate" its usage for more complex streams, right?

@mstade
Copy link
Member

mstade commented Feb 22, 2019

Or, the same but in other words: keep reducers on the store config unless we need to compose the streams into more ... symphonic arrangements?

(Emphasis mine.)

@gabrielmontagne, this is why I love you and that brilliant mind of yours.

@gabrielmontagne
Copy link
Contributor Author

@cristiano-belloni , I think so, yes.

@mstade , good to see you in the neighbourhood :-)
I hope once we sketch what this is all about in some adoc file, you'll feel tempted to take this stuff for a spin.

thinking out loud, the operator name could be stow (or place or sit) .. ?

@cristiano-belloni cristiano-belloni changed the title on keeping all the reducing functions on the store module on keeping all the reducing functions on the store module aka stow operator Mar 22, 2019
@cristiano-belloni
Copy link
Collaborator

+1 for the stow operator, especially now that we can have a bunch of branches which cook their partial state and we can just stow the slice of state they provide instead of ending them with a non-reusable reducer.

@gabrielmontagne
Copy link
Contributor Author

Will create a quick PR with the implementation above. No need to get fancier for the time being, I think.

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

Successfully merging a pull request may close this issue.

3 participants