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

Proof of concept of deferred props #24

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sergesemashko
Copy link

This a proof of concept for having deferred props.

Usage

Pass deferred props as a 2nd parameter to @asyncConnect:

@asyncConnect({...props}, {...deferredProps})

Under the hood

  • Using server-side rendering:
    • Async props are loaded as usual by server
    • Deferred props are initialized with initial state {loading: false, loaded: false}
    • Client loads deferred props on componentDidMount
  • Without server-side rendering:
    • Non-server environment loads deferred and default props all together

TODO:

  • update the docs once the code is finalized

@sars, let me know what do you think

@sars
Copy link
Member

sars commented Feb 1, 2016

@sergesemashko Thank you, for this PR,
we actually discussed this approach here:
erikras/react-redux-universal-hot-example#872 (comment)
Is it what you are trying to solve?

To be honest, i don't really like this approach - I mean new static method and so on...
trying to find more elegant solution....

@sergesemashko
Copy link
Author

Thanks for the feedback, @sars. I understand. But I would like to know if there are more specific concerns about static methods, etc. or your vision how it would be better to implement this feature.
Let's list points or concerns and work towards them. I would like to help to make deferred props happen in this project.
Thanks

@krzysztofpniak
Copy link

Hey, any news when async props can be available here?
Without them I do not really see benefit of having loading and loaded properties in store.

@sars
Copy link
Member

sars commented Feb 11, 2016

@krzysztofpniak I'm working on v1.0.0
there will be several changes there including deferred props
should be ready in 1-2 days

@sars
Copy link
Member

sars commented Feb 12, 2016

@sergesemashko, @krzysztofpniak , take a look please:
https://github.com/Rezonans/redux-async-connect/tree/v1

there is new interface for @asyncConnect decorator in this version.

Now you can use it like:

@asyncConnect([
  {key: 'lunch', promise: ({params, helpers}) => helpers.client.get('/lunches/' + params.lunchId), group: 'smth'}
])

and then in your client.js:

const component = (
  <Router render={(props) =>
        <ReduxAsyncConnect {...props} filter={item => item.group === 'smth'} helpers={{client}} />
      } history={browserHistory}>
    {getRoutes(store, client)}
  </Router>
);

and server.js:

loadOnServer({...renderProps, store, helpers: {client}}).then(() => {
  // ...
}

Does it works for you?

@sergesemashko
Copy link
Author

hey @sars, sorry for the delay. I assume there should an additional filter for server:

loadOnServer({...renderProps, store, helpers: {client}}).then(() => {
  const appHTML = renderToString(
        <Provider store={store} key="provider">
          <ReduxAsyncConnect filter={item => item.group !== 'deferred'} {...renderProps} />
        </Provider>
      )
}

Right?

From the first look, this way of defining deferred props not too obvious and requires patching of the application with filter. It would make sense to make it more explicitly, I think.

Overall, it should work. Just curious, what are the other purposes of filters aside deferred props?

Thanks

@mmahalwy
Copy link

@sars is the purpose of the array suppose to be sequential?
Also, is the static method going away?

@sars
Copy link
Member

sars commented Feb 18, 2016

@mmahalwy
no, all promises invokes simultaneously like in previous version
static method still can be used, but it looks much inconvenient now, it's interface changed as well.

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 this pull request may close these issues.

4 participants