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

Connecting to both Redux and non-Redux store #713

Closed
aikoven opened this issue Dec 21, 2016 · 10 comments · Fixed by #735
Closed

Connecting to both Redux and non-Redux store #713

aikoven opened this issue Dec 21, 2016 · 10 comments · Fixed by #735

Comments

@aikoven
Copy link
Contributor

aikoven commented Dec 21, 2016

I have some stuff that I can't put into Redux store because it is not serializable and not immutable. More precisely, a Connection object that is created and managed by a third-party lib. To avoid passing connection via Redux action, I use Channels. It works quite well, despite the need to pass channels to every saga that needs the connection.

However, a lot of generators aren't started on application startup, so they don't have a chance to catch the connection from channel. To workaround this, I have to pass connection along with channel to lots of generators which is not convenient at all.

I have one idea how to solve this. It's a common practice to assign third-party clients to Redux store like this:

const store = createStore(...);
store.thirdPartyClient = new ThirdPartyClient(...);

Then you're able to use the client in React components.

Saga runtime is connected to the store, although only to getState and dispatch. But if we connect runtime to the whole store, we could introduce custom effect creators like this:

function getConnection() {
  return store => store.thirdPartyClient.connection;
}

And then use it in any generator without the need to pass connection as argument:

const connection = yield getConnection();

Maybe there's a more elegant way, like introducing something similar to React Context.

@Andarist
Copy link
Member

Andarist commented Dec 21, 2016

Effect creator is a function returning a description-object, look at this.

Also the created effects are later interpreted here.

So for allowing custom effects we would need to expose hooks to those internals and document how to work with it, might make sense for other use cases, but for yours it seems like an overkill.

Im wondering - isnt just importing an external module containing a reference to your connection viable option?

Also - the title of the issue implied for me that this could be used.

@aikoven
Copy link
Contributor Author

aikoven commented Dec 21, 2016

Effect creator is a function returning a description-object, look at this.

Also the created effects are later interpreted here.

Yep, I know, and I agree that it's probably an overkill for this problem, however I still think that custom effects is a feature that should be added one day. It would be useful for many things. Currently you can code it as a composition of built-in effects, e.g.

function safeCall(...args) {
  return call(function* safeWrapper() {
    try {
      return yield call(...args);
    } catch (e) {
      return null;
    }
  });
}

But it won't look nice in the monitor and there's no way for it to access saga runtime stuff.

Im wondering - isnt just importing an external module containing a reference to your connection viable option?

I'm trying to refactor some code that uses this approach to something that won't involve global variables. These connections are not singletons and are created and destroyed often.

React Context allows components to access objects owned by their far ancestor, while avoiding globals and passing objects down the whole hierarchy via props. This makes it possible to attach a Redux store once near the root component and then use it in any other descendant. I think this kind of inversion of control would be really useful in sagas, because they too often form a deep hierarchy.

Also - the title of the issue implied for me that this could be used.

I want to still be able to put to Redux along with accessing non-redux store. I think it's possible to build some smart dispatch and getState options that would somehow route actions and state to the correct store. Seems fragile though.

@Andarist
Copy link
Member

But it won't look nice in the monitor and there's no way for it to access saga runtime stuff.

Out of curiosity - what saga runtime stuff you would like to have access to?

Also - the title of the issue implied for me that this could be used.

I want to still be able to put to Redux along with accessing non-redux store. I think it's possible to build some smart dispatch and getState options that would somehow route actions and state to the correct store. Seems fragile though.

But would abstract your connections and you wouldnt need to care about it in your sagas

@aikoven
Copy link
Contributor Author

aikoven commented Dec 22, 2016

Out of curiosity - what saga runtime stuff you would like to have access to?

Maybe the options passed to middleware, or arguments of root saga.

But would abstract your connections and you wouldnt need to care about it in your sagas

I would still need to care about it, by putting some special kind of actions and selecting in some other non-trivial way. Things would get even worse when I have to use several instances of different third-party clients. Overriding dispatch and getState is a hack and doesn't scale.

@Andarist
Copy link
Member

Well, I guess allowing custom effects wouldnt be even that much of a problem, only if they are self contained and do not need access to the proc's closure. Although I dont see much of the use case for that beside looking more nice in the monitor and allowing ofc access to some external objects without needing to pass them around as args (so basically a context)

WDYT @yelouafi ?

@aikoven
Copy link
Contributor Author

aikoven commented Dec 22, 2016

After some more thinking I agree that custom effects are actually useful only for Monitor. But I guess there are simpler ways of making them look nice, while still having them composed of built-in effects.

Introducing context seems like a better option to me. Something like this maybe?

function* parent() {
  yield setContext({foo: 'bar'});

  yield fork(child);
}

function* child() {
  const foo = yield getContext('foo');  // 'bar'
}

@Andarist
Copy link
Member

does React's context allows removal?

@aikoven
Copy link
Contributor Author

aikoven commented Dec 22, 2016

When parent component is updated, it can return new context object each time. You only specify how context is computed from component's props and state, not setting it imperatively.

We could make it more minimalistic, allowing only getContext effect, and setting context globally:

const sagaMiddleware = sagaMiddlewareFactory({context: {...}});
// or
sagaMiddleware.setContext({...});

@yelouafi
Copy link
Member

Seems like the concept dynamic scoping. I think it could be helpful for things like dep. injection.

This could be implemented using prototype chaining of JS objects. We can pass an additional arg to proc which is the parent context (context of the caller/forker saga), then we create the actual context using Object.create(parentContext). This way getContext/setContext could be implemented using simple JS object property access (a question is should getContext throw if the var. can't be resolved)

@aikoven
Copy link
Contributor Author

aikoven commented Feb 12, 2017

Closing in favor of #735.

@aikoven aikoven closed this as completed Feb 12, 2017
Andarist added a commit that referenced this issue Feb 13, 2017
…amic scoping between parent/child tasks (resolves #713)
Andarist added a commit that referenced this issue Feb 24, 2017
…amic scoping between parent/child tasks (resolves #713)
Andarist added a commit that referenced this issue Apr 24, 2017
…amic scoping between parent/child tasks (resolves #713)
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