makeomatic / redux-connect

Provides decorator for resolving async props in react-router, extremely useful for handling server-side rendering in React
MIT License
549 stars 67 forks source link

Handle "asyncConnect" decorated containers that are hidden in the children components #45

Open elodszopos opened 8 years ago

elodszopos commented 8 years ago

I have a nested structure of components (containers). Each has it's own asyncConnect. The top one can hold up to N number of children. These children can come from a CMS. The schema is well defined and each maps to one of these children.

Let's say the top level one would load available categories in an API call. Then the children ( sub-categories ) can be included dynamically, and each would worry about it's own content, separately, independent of the top level container ( other than a route parameter that contains the id of the category ). Each of these children would load details for the sub-category it is responsible for.

Some pseudo code below:

<Route ... component={TopLevelContainer} />

@asyncConnect([{
  promise: ({ store: { dispatch, getState }, params: { categoryId } }) => {
    const promises = [];
    const state = getState();

    if (!categoryActions.isLoaded(state)) {
      promises.push(categoryActions.loadCategory(categoryId));
    }

    .... 
    return Promises.all(promises);
}])
export default class TopLevelContainer extends Component { ....
    someChildContext stuff ...
    ....
    render() {
        return (
            <div>
                <ChildContainer />
                <ChildContainer />
            </div>  
        );
    }
@asyncConnect([{
  promise: ({ store: { dispatch, getState }, params: { categoryId } }) => {
    const promises = [];
    const state = getState();

   // why is this code not running ? Am I forced to keep this on the parent? 

    // COMMENT BELOW
    if (!subCategoryActions.isLoaded(state, categoryId, 'someSubcategory')) {
      promises.push(subCategoryActions.loadCategory(categoryId, 'someSubcategory'));
    }

    .... 
    return Promises.all(promises);
}])
export default class ChildContainer extends Component { ....

So .. I know it's a bit of code, but I was using the repo that this was forked from in hopes that maybe this would work here. And yeah .. am I doing something wrong? Is this a feature that's implemented and support and I'm not doing something correctly? I also found this on the older repo but I'm not sure it's what I need.

Also, regarding the // COMMENT BELOW part, is it somehow possible to get the context in the async connect? Or is that a more general thing?

Any help would be greatly appreciated, Thank you very much!

AVVS commented 8 years ago

Components are gathered form react-router match function, therefore redux-connect doesn’t see any nested components. All the components with asyncConnect must be defined on the router, everything else won’t work. You can nest them IN react-router as much as you want, but they must be in the router. No async dynamic bananza here.

<Route path=“/nice“ component={Top} >
   <Route path=“/nice/:id” component={Children} />
</Route>

Hope I answered your question :)

On Jul 4, 2016, at 11:13 PM, Elod-Arpad Szopos notifications@github.com wrote:

I have a nested structure of components (containers). Each has it's own asyncConnect. The top one can hold up to N number of children. These children can come from a CMS. The schema is well defined and each maps to one of these children.

Let's say the top level one would load available categories in an API call. Then the children ( sub-categories ) can be included dynamically, and each would worry about it's own content, separately, independent of the top level container ( other than a route parameter that contains the id of the category ). Each of these children would load details for the sub-category it is responsible for.

Some pseudo code below:

<Route ... component={TopLevelContainer} />

@asyncConnect([{ promise: ({ store: { dispatch, getState }, params: { categoryId } }) => { const promises = []; const state = getState();

if (!categoryActions.isLoaded(state)) {
  promises.push(categoryActions.loadCategory(categoryId));
}

.... 
return Promises.all(promises);

}]) export default class TopLevelContainer extends Component { .... someChildContext stuff ... .... render() { return (

    );
}

@asyncConnect([{ promise: ({ store: { dispatch, getState }, params: { categoryId } }) => { const promises = []; const state = getState();

// why is this code not running ? Am I forced to keep this on the parent?

// COMMENT BELOW
if (!subCategoryActions.isLoaded(state, categoryId, 'someSubcategory')) {
  promises.push(subCategoryActions.loadCategory(categoryId, 'someSubcategory'));
}

.... 
return Promises.all(promises);

}]) export default class ChildContainer extends Component { .... So .. I know it's a bit of code, but I was using the repo that this was forked from https://github.com/Rezonans/redux-async-connect in hopes that maybe this would work here. And yeah .. am I doing something wrong? Is this a feature that's implemented and support and I'm not doing something correctly? I also found this on the older repo https://github.com/Rezonans/redux-async-connect/issues/47 but I'm not sure it's what I need.

Also, regarding the // COMMENT BELOW part, is it somehow possible to get the context in the async connect? Or is that a more general thing https://github.com/reactjs/react-redux/issues/289?

Any help would be greatly appreciated, Thank you very much!

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/makeomatic/redux-connect/issues/45, or mute the thread https://github.com/notifications/unsubscribe/ABol0T9_Gvq17mlrL6GMHwnhNOTCHP4fks5qSWlogaJpZM4JEodF.

elodszopos commented 8 years ago

But .. but .. CMS is all about dynamic bananza :(

Thanks for the explanation @AVVS. Figured this out shortly after I wrote the post. Before you close this out: which basket should I put my eggs in? Will you be more actively sustaining the fork?

AVVS commented 8 years ago

I'm maintaining in, so far we really have 1 use-case thats not covered - partial load from server and then loading the rest on the client. Unless you need it in the short term - this is a good module to use.

Other good alternatives are redial and redux-saga. Though they dont have what you want as well :)

On 05 Jul 2016, at 07:53, Elod-Arpad Szopos notifications@github.com wrote:

But .. but .. CMS is all about dynamic bananza :(

Thanks for the explanation @AVVS. Figured this out shortly after I wrote the post. Before you close this out: which basket should I put my eggs in? Will you be more actively sustaining the fork?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

elodszopos commented 8 years ago

I'm keeping my eggs over here! Keep up the good work, thank you very much for the quick assistance @AVVS

sergesemashko commented 8 years ago

hi @AVVS, myself and my team are actively using this module in projects and after certain experience I can say that I would love nested components support. Our team noticed that often single asyncConnect function that fetches data for entire page is getting bloated and it's hard to reuse fetched data or fetching logic. In most cases it's not possible to split asyncConnect promise functions because of data dependencies.

Redux-saga(my opinion, fairly complicated) and redial are good alternatives, but they both always require custom reducers, even for repeating simple fetches. Redux-connect takes that off from developers and just uses single reducer making data fetching easy.

I'm going to play around and prototype something like this https://github.com/ericclemmons/react-resolver/blob/master/src/resolve.js anyways, just wanted to know is it welcomed to apply that kind of update? thanks

elodszopos commented 8 years ago

I whole-heartedly support that idea and if you need any help or support @sergesemashko, feel free to drop a line here. Granted @AVVS also agrees.

I was on the verge of going for it myself but unfortunately I had little time as of late.

AVVS commented 8 years ago

@sergesemashko you actually can use multiple asyncConnect functions for any number of components, if they are nested - they will resolve in series (to support state that depends on previous state), if they are in parallel branches - they will resolve in parallel. Key part here is that all asyncConnect components are all defined on react-router and are resolved from it during match action (even if it's a dynamic resolve). Given that nesting is actually supported.

What's not supported is a component, which renders a child component and that child component is actually a container with asyncConnect attached and it was not visible anywhere before that render() call in react occurred. In that case there is no way to find that static property on the container beforehand.

Given you have all that insight on that issue now - if you have any idea how to do nesting before rendering with no state - I'd fully support it and will help implement it. However, I have no idea how to achieve that as of this moment and therefore not tackling that issue at all at this moment

sergesemashko commented 8 years ago

glad to hear your sign off. @AVVS, thanks, that makes things even more clearer. React-resolver does nested fetching using react context + flags and data fetching on render. 2nd option would mean breaking change to redux-connect server rendering. I'll give it a shot and ping you back when I come up with something

AVVS commented 8 years ago

@sergesemashko let me know once you have more insight on this :)

oyeanuj commented 8 years ago

@sergesemashko any updates here by any chance?

sergesemashko commented 8 years ago

@oyeanuj, yes, I think I got pretty close, just would like to solve some obvious issues before submitting a PR.

DanilloCorvalan commented 8 years ago

I think this is going to fix a problem with high order components, for instance, to control login permissions, redirects, etc.. When doing something like

<Route component={RequiresLogin(AdminView)} />

AdminView uses AsyncConnect, but because of that RequiresLogin can't render it.

AVVS commented 8 years ago

@DanilloCorvalan you can make your HOC copy static over if it's present - then it would be visible.

DanilloCorvalan commented 8 years ago

@AVVS thanks for the fast reply :) It gets visible but asyncConnect doesn't trigger the promise.

oyeanuj commented 8 years ago

@AVVS, one clarification please. In your above comment, you say:

Key part here is that all asyncConnect components are all defined on react-router and are resolved from it during match action (even if it's a dynamic resolve). Given that nesting is actually supported.

What would be an example of that?

@sergesemashko thanks for the update. I am curious about how you are thinking of resolving it - how would it work in your solution?

(My use-case, FWIW, is a component that contains of multiple other components as children, all of which have asyncConnect)

sergesemashko commented 8 years ago

@oyeanuj, I'm trying to introduce as less breaking changes as possible. My current solution for server-side rendering has recursive call of renderToStaticMarkup() to handle sub-children asyncConnect decorators, like:

function loadOnServer(render, { store, filter = () => true, ...rest }) {
    const initialLoadState = store.getState().reduxAsyncConnect.loadState;
    const queue = [];
    // onResolve triggered on first unresolved deeply nested asyncConnect. 
    // If there are multiple asyncConnect on the same tree level queue will have multiple promises.
    renderToStaticMarkup(
      <Provider store={store} key="provider">
        <AsyncConnectResolver
          {...rest}
          filter={ filter }
          onResolve={(promise) => queue.push(promise)}
        >
          { render }
        </AsyncConnectResolver>
      </Provider>
    );

    return Promise.all(queue).then(results => {
      const loadState = store.getState().reduxAsyncConnect.loadState;
      // @TODO: Solution is taken from react-resolver, investigate better options. 
      // If load state hasn't change on last render that means there is no asyncConnect 
      // on deeply nested children left and we can leave the recursion.
      if (Object.keys(initialLoadState).length < Object.keys(loadState).length) {
        return loadOnServer(render, { store, filter, ...rest });
      }
      //@TODO: decide what to return here. Basically, reaching this point means all async connect are resolved;
      return {};  
    });
  }

Client-side solution is still in progress, but I'm trying out to have listener on each component wrapped by asyncConnect to handle its deeply nested children. So, let's say we have:

<ReduxAsyncConnect>
  <AsyncConnectContainer>
    <AsyncConnectedChild>
      <AsyncConnectedSubChild />
    </AsyncConnectedChild>
  </AsyncConnectContainer>
  <SecondAsyncConnectContainer />
</ReduxAsyncConnect>

Resolving pipeline for given tree will be:

  1. ReduxAsyncConnect fires beginGlobalLoad() and listens for AsyncConnectContainer and SecondAsyncConnectContainer promises are resolved.
  2. AsyncConnectContainer is going to listen wether AsyncConnectedChild is resolved.
  3. AsyncConnectedChild is going to listen for AsyncConnectedSubChild and once AsyncConnectedSubChild is resolved, promise resolve is bubbling up to AsyncConnectedChild->AsyncConnectContainer->ReduxAsyncConnect
  4. Once two implicit children of ReduxAsyncConnect are resolved it fires endGlobalLoad()
KATT commented 8 years ago

Just my two cents from my experience with Relay. They solve a similar problem with their fragments. The containers must then know if they have sub-containers and ask about their data requirements using .getFragment().

This approach would allow for the requests to be executed in parallell (or in Relay, it's actually all joined into one request). You wouldn't need to multiple re-renders on the server either.

Not familiar with this codebase to give input, but essentially the containers would need to recursively ask sub-containers for their reqs.

AVVS commented 8 years ago

@KATT currently it's not possible to know a complete list of child containers before calling render, that being said we might add an extra static method, such as getChildContainers(), which is supposedly returns a list of child containers and then the responsibility for making that function lies on the app creators. However, I don't see this approach as particularly amusing, as it would require plenty of "manual" coding and hurt composability

KATT commented 8 years ago

Yeah, there is a bit of overhead to create all the containers in Relay and making sure everything is a container that can pass up it's data reqs to the HOC. It's a bit annoying to honest.

However, resolving it dynamically, rather than statically like Relay does, means staggered initial load time for the containers further down in the tree. There's pros and cons.

sergesemashko commented 7 years ago

sorry for not responding for a long time. I got to the point where fetch for nested components happens only when they appear in render() call on client, on server I managed to do it by recursive React.renderToString() calls until all nested promises are resolved. I don't know how to prevent flush to DOM and still call render() to trigger nested components on client. If anyone has any ideas, please share. So, fetch for nested components is always deferred and happens after page transition on client. @AVVS, do you think it's worth continuing in that path?

AVVS commented 7 years ago

I don't really like the solution, because it does not resolve state before rendering, but instead renders to collect component's tree and fetchers and then re-render it again. It's inefficient and can be avoided by simply deferring to loading data at the router level. I've completed a few big projects with that approach and I can't say it's hard to do

sergesemashko commented 7 years ago

@AVVS, I see and agree, I think we should call it "the end" of the issue unless anyone has strong arguments or willing to give another try :) sounds good?

AVVS commented 7 years ago

@sergesemashko sure :)