Added getContext/setContext effects, where context is acting like dyn… #735

Andarist · 2016-12-31T01:11:19Z

…amic scoping between parent/child tasks (resolves #713)

Just a quick draft of this proposed API. I've put several TODO:s in the code, they act as thought-placeholders and I would appreciate commenting each one of them.

Also there I've spotted a difference between standard middleware API and runSaga.

middleware - options are passed once, so encapsulated by the factory) and each subsequent middleware.run(saga) will receive them (also context now)
runSaga - each call expects the options all over again, so subsequent calls do not share context and it cannot be shared among 'top sagas' (at least not without creating a topContext within runSaga module)

cc @aikoven

Andarist · 2017-01-01T00:58:31Z

also I think like some way for removing the context should be available as for now it wouldnt be possible to 'uncover' higher scope

function* saga() {
	yield setContext({ a: 1 })
	yield fork({
	  yield setContext({ a: 2 })
	  yield setContext({ a: undefined })
	  yield getContext('a') // undefined, no way to reach { a: 1 } at this point
	})
}

hating to put 3 effects for this rather simple feature - getContext, setContext and unsetContext (?)

aikoven · 2017-01-01T06:59:26Z

First of all, thanks for spending time on this.

Some thoughts:

Should generator have access to context properties set by itself? IMO it shouldn't, because you can just use variables for this. So maybe setContext should only have effect on children? Maybe even rename setContext with setChildContext (I don't really like the verbosity though)?
For removing context we can make setContext replace all own properties instead of assigning. This way we can keep API surface smaller.
Speaking about uncovering, consider this:

function* a() {
  yield setContext({foo: 1});
  yield fork(function* b() {
    yield setContext({foo: 2});
    yield fork(function* c() {
      // ..
    });
  });
}

Should c be able to uncover context set in its grandparent a? I think not, because it will break encapsulation.

aikoven · 2017-01-01T07:03:06Z

src/internal/proc.js

+    // TODO: if object returned -> return shallow copy?
+    // should yield getContext() be available to access context?
+    // if yes then shallow copying it is a must to avoid unexpected mutations
+    cb(taskContext[prop])


Context property could be anything, not just plain object, so we can't safely copy it.

Also, I think we should throw if property doesn't exist in context to distinguish between the case where context doesn't have a property and the case where context has property with value undefined.

eh, you static typing guys and your habits! 😄

Context property could be anything, not just plain object, so we can't safely copy it.

ofc it would be checked beforehand if it is a plain object to see if we should make a shallow copy

Ok, I tend to agree with you that throwing is not really necessary here as any relevant checks could be easily performed on the user side.

Andarist · 2017-01-01T11:31:30Z

Should generator have access to context properties set by itself? IMO it shouldn't, because you can just use variables for this. So maybe setContext should only have effect on children? Maybe even rename setContext with setChildContext (I don't really like the verbosity though)?

I dont think it will be a problem as this context feature is less convenient to use, so people will always prefer accessing variables than this within task which uses setContext

For removing context we can make setContext replace all own properties instead of assigning. This way we can keep API surface smaller.

I think that could lead to unexpected bugs and also updating the context would be quite unwieldy

Should c be able to uncover context set in its grandparent a? I think not, because it will break encapsulation.

Nah, didnt think about uncovering like this, just 1 step into the upper context like in the example posted here

aikoven · 2017-01-05T06:57:46Z

For removing context we can make setContext replace all own properties instead of assigning. This way we can keep API surface smaller.

I think that could lead to unexpected bugs and also updating the context would be quite unwieldy

Another way to avoid unsetContext is to use symbols for context keys on the library user's side.

Context property could be anything, not just plain object, so we can't safely copy it.

ofc it would be checked beforehand if it is a plain object to see if we should make a shallow copy

Is this really a library responsibility? We don't shallow copy selected state and actions returned from take.

Andarist · 2017-01-05T09:48:52Z

Context property could be anything, not just plain object, so we can't safely copy it.

ofc it would be checked beforehand if it is a plain object to see if we should make a shallow copy

Is this really a library responsibility? We don't shallow copy selected state and actions returned from take.

Good point, however I feel like context has a higher probability of misusing the feature. With state it is not that much of a problem as there is quite strong mindset that updating the state happens via action + mutating the state breaks connect, so people in general already know that they shouldnt do this.

aikoven · 2017-01-15T14:12:02Z

Here's a counterexample:

const obj = {
  foo: null,
  setFoo(foo) {this.foo = foo},
  getFoo() {return this.foo}
};

yield setContext({obj});
// ...
const obj1 = yield getContext('obj');
yield call([obj1, obj1.setFoo], 1);
// ...
const obj2 = yield getContext('obj');
const foo = yield call([obj2, obj2.getFoo]);  // null

Here, the value of obj.foo is lost due to shallow copying. In real world, this could happen when you store observable objects in context and later add callbacks to them.

Andarist · 2017-01-15T14:25:36Z

Hm, interesting. Its really a scary situation and compelling one at the same time to drop shallow copying. Wondering only which usage would be more often and therefore which approach would give us less footguns in the end.

In theory we could make 2 versions of this effect getContext and getContext.impure

In general im in favor of immutability and this-less programming, aint saying though that everyone should. This feature would be experimental anyway and rather an escape hatch which should noy be used in trivial cases anyway, so... i dont know which is better now as a default

aikoven · 2017-01-15T14:58:53Z

In general im in favor of immutability and this-less programming

I absolutely agree with you, but my reasoning is this: for pure things we already have Redux store and actions which we can access in sagas at any level using appropriate effects. We would use Context to access non-pure third-party library objects.

Another use case is to share Channels between sagas, however I guess shallow-copying a Channel would be safe.

yelouafi · 2017-01-23T09:33:27Z

src/internal/proc.js

+        subscribe,
+        dispatch,
+        getState,
+        (detached ? null : taskContext), // TODO: topContext should be passed in case of spawn?


why cant we just pass the parent context?

reasoning behind it - spawn creates a detached task, which is acting like a top task, so would it be good to propagate taskContext (parent context) to it? behaviour would differ from other propagations (error/cancellation) in terms of detached (spawn) and non-detached tasks (fork)

yeah but you could also view spawn as a simple child task with different behavior regarding cancellation/errors? And also with analogy to lexical scope, it gets its params from its parent task, so could be the same from dynamic scope (context)

could be ofc, but what i have proposed felt more natural when i was implementing this, however that was just a question for reviewers (so mostly you :P) left in the source code

also a little bit connected is this - #763 (comment)

in regards of spawn/fork debate, cancel propagation and maybe even need to distinguish rootTask from regular tasks for code-splitting purposes and proper cancelation logic on route changes, would appreciate your input about those matters there greatly!

Agree with @yelouafi, if we disable propagating context on spawn, it would make context a much less convenient mechanism for dependency injection.

That said, I think that runSaga should behave just like you said — starting with an empty context by default. Another reason is that otherwise we'd have to somehow link runSaga with sagaMiddleware instance.

aikoven · 2017-02-12T06:54:14Z

src/internal/io.js

+}
+
+export function setContext(props) {
+  // TODO: check if props is object/plain object?


Yep, I think we should restrict props to be a plain object.

aikoven · 2017-02-12T07:06:05Z

src/internal/middleware.js

@@ -91,5 +93,14 @@ export default function sagaMiddlewareFactory(options = {}) {
    return task
  }

+  sagaMiddleware.setContext = (props) => {


What's the purpose of this API? I can see cases where you need to change the root context as a result of some event, but I think it's more convenient to do this in the root saga.

Just found out this was proposed by myself: #713 (comment)

Although I think only one of options.context or sagaMiddleware.setContext() is enough.

well, one is needed upon creation and latter might be used in a runtime, could be ofc in response for some action but for more complex contexts it would require action to contain ur dependency which could be not serializable fully, which i dont like - if you for example need to pass some event bus with methods on it

Good point. 👍

aikoven · 2017-02-12T07:07:48Z

src/internal/middleware.js

+    // TODO: or maybe `setContext` should be available on task and this one should act only as proxy method?
+    // would that be useful?
+    // const task = yield takeEvery(...)
+    // const task.setContext({...}) or maybe even - yield setContext(task, {...})?


IMO the context of a task should be a private thing, just like state in React components. The component itself is an owner of its state and it shouldn't be accessible from its parent.

actually as React's setState is available on the instance of the component - it could be used outside and therefore its not so private

Yep, also I can see some parallel with sagaMiddleware.setContext. Maybe then we just leave task.setContext and remove sagaMiddleware.setContext, as the latter can be done with sagaMiddleware.run(...).setContext?

hm... at first I thought that this is a valid point and should be done, but after a while i think in the world of code-splitting its nice to have a possibility to share this top-level middleware context among root sagas, as there could be many of those, and when puting this only on task itself it would require such verbose code in order to share it 'globally':

const context = {...} sagaMiddleware.run(...).setContext(context) // ... later sagaMiddleware.run(...).setContext(context)

You're right, I've overlooked the case when sagaMiddleware.run is called multiple times.

the only thing im not so happy about is that sagaMiddleware is kinda superior to the runSaga here, as the latter do not have possibility to share such things in an elegant way, but I guess one can always write some wrapper function up to his needs

aikoven · 2017-02-12T07:17:13Z

src/internal/proc.js

+  }
+
+  function runSetContextEffect(props, cb) {
+    Object.assign(taskContext, props)


A check for props being a plain object should be added here as well.

such checks are usually performed in effect creators, it would only double the same check if we put it here also, as its not possible to reach this - runSetContextEffect in any other way that yield setContext(...)

Andarist · 2017-02-12T11:30:58Z

src/internal/io.js

@@ -153,6 +155,16 @@ export function flush(channel) {
  return effect(FLUSH, channel)
 }

+export function getContext(prop) {
+  check(prop, is.string, `getContext(prop): argument ${ prop } is not a string`)
+  return effect(GET_CONTEXT, prop) // TODO: maybe accept functions also?


cc @aikoven

Since we do Object.assign and not deep merge, I'd view context as a flat object. Thus not sure if complex accessors are needed. Maybe I'm missing some use cases, but I'd rather add such features on demand.

Andarist · 2017-02-13T10:35:54Z

@aikoven
Last comments made I think (ofc still waiting for your input, if you disagree - or agree - somewhere) and I think im gonna update the PR soon so it can get finally merged in

EDIT:// heh, while writing this you have already replied 👍

aikoven

I guess we have a consensus finally 😉

Andarist · 2017-02-13T23:02:47Z

I think the code is final now, so you can have last look on it - whats missing before merging is some tests + docs, ill get them done in this week and merge whole thing to the master.

aikoven · 2017-02-24T09:33:54Z

src/internal/middleware.js

 import proc from './proc'
 import { emitter } from './channel'
 import { ident } from './utils'

-export default function sagaMiddlewareFactory(options = {}) {
+export default function sagaMiddlewareFactory({ context = {}, ...options }) {


Won't this break if we call factory without arguments?

it would, thanks for noticing! gonna fix it with default param in a minute

AriaMinaei · 2017-03-08T17:11:33Z

Guys, do you think this branch is going to get merged eventually?

Andarist · 2017-03-08T17:44:54Z

@AriaMinaei yes, it sure will! I just need to get some time to merge it and some other branches, so I can release new version, sorry about the delay

neurosnap · 2017-04-21T12:46:06Z

Is there anything I can do to help out with this PR?

Andarist · 2017-04-21T13:39:39Z

@neurosnap, i think its basically done. Will review it again in the weekend and gonna publish

…amic scoping between parent/child tasks (resolves #713)

Andarist · 2017-04-24T11:51:13Z

Released, not docs yet - but if you know about the feature and want to use it - go ahead. Let me know if everything works as expected.

Andarist requested a review from yelouafi December 31, 2016 01:11

aikoven reviewed Jan 1, 2017

View reviewed changes

Andarist mentioned this pull request Jan 14, 2017

Pattern: Saga Processses - Feedback Requested #761

Closed

Andarist mentioned this pull request Jan 17, 2017

Take last action that matches pattern #768

Closed

yelouafi reviewed Jan 23, 2017

View reviewed changes

Andarist mentioned this pull request Feb 10, 2017

Update TypeScript definitions #740

Merged

aikoven reviewed Feb 12, 2017

View reviewed changes

aikoven mentioned this pull request Feb 12, 2017

Connecting to both Redux and non-Redux store #713

Closed

Andarist commented Feb 12, 2017

View reviewed changes

aikoven approved these changes Feb 13, 2017

View reviewed changes

Andarist force-pushed the context branch from c6032cf to f35dd4b Compare February 13, 2017 23:00

aikoven reviewed Feb 24, 2017

View reviewed changes

Andarist force-pushed the context branch from f35dd4b to d4eccae Compare February 24, 2017 10:53

Added getContext/setContext effects, where context is acting like dyn…

69dd9cb

…amic scoping between parent/child tasks (resolves #713)

Andarist force-pushed the context branch from d4eccae to 69dd9cb Compare April 24, 2017 11:16

Andarist merged commit 69dd9cb into master Apr 24, 2017

Andarist deleted the context branch April 24, 2017 11:51

aikoven mentioned this pull request Apr 28, 2017

Writing sagas based on context services instead of singletons #939

Closed

aikoven mentioned this pull request Jun 6, 2017

Share data between sagas #1013

Closed

renovate bot mentioned this pull request Feb 2, 2018

Update dependency redux-saga to ^0.16.0 JimmyLv/Haiku#14

Open

1 task

rclmenezes mentioned this pull request Feb 6, 2019

setContext inside fork doesn't propagate to other sagas #1765

Closed

Added getContext/setContext effects, where context is acting like dyn… #735

Added getContext/setContext effects, where context is acting like dyn… #735

Conversation

Andarist commented Dec 31, 2016 • edited Loading

Andarist commented Jan 1, 2017

aikoven commented Jan 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Andarist Jan 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Andarist commented Jan 1, 2017

aikoven commented Jan 5, 2017

Andarist commented Jan 5, 2017

aikoven commented Jan 15, 2017

Andarist commented Jan 15, 2017

aikoven commented Jan 15, 2017

yelouafi Jan 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Andarist commented Feb 13, 2017 • edited Loading

aikoven left a comment

Choose a reason for hiding this comment

Andarist commented Feb 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AriaMinaei commented Mar 8, 2017

Andarist commented Mar 8, 2017

neurosnap commented Apr 21, 2017

Andarist commented Apr 21, 2017

Andarist commented Apr 24, 2017

Andarist commented Dec 31, 2016 •

edited

Loading

Andarist Jan 1, 2017 •

edited

Loading

yelouafi Jan 23, 2017 •

edited

Loading

Andarist commented Feb 13, 2017 •

edited

Loading