-
Notifications
You must be signed in to change notification settings - Fork 320
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
A proposal for render #393
Conversation
Sorry I've been reading through this in parts but I'm jet lagged/feeling like crap - First question is that I notice you still separate out the "remote get" logic from the "local get" logic, i.e. |
@@ -0,0 +1,149 @@ | |||
import Alt from './' | |||
import React from 'react' | |||
import Render, { connect } from './utils/Render' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You shouldn't be able to import context
connect
like that - this is only working because of Babel's fallback for when you have a single default export.
EDIT: Fixed typo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, fixed this. Will push update soon.
Regarding resolve vs reduce, yes they need to be separated. Resolve is just about resolving a set of promises before the component is even rendered. Reduce is about what gets sent down to the component. The key here is flexibility. Often though you may just want to resolve all the props and send them all down. In that case you can write easy getters for that task. |
I still think it almost always makes more sense to combine the two. I'll prototype something up real quick. How do I, uhh, run this? I get |
|
||
// this takes the rendering out of a Promise context | ||
// TODO test this concurrently | ||
return setTimeout(() => this.render(alt, Element, info)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something like
return Promise(resolve => {
setTimeout(() => resolve(this.render(alt, Element, info)))
});
no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, fix this line in order to fix the error above!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this isn't necessary actually, im doing this to have a better separation because if a react component throws it'll instead be handled by the Promise which I don't want it to be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well the problem is that you resolve the promise with the time out rather than something that unwraps into the expected object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right, that's silly. What you wrote above should work.
I don't think they should be combined because they're two different things. Combining them would end up looking a lot like react-resolver/react-transmit but with a flux backing. Plus this complicates a few things, like whenever a store changes we have to keep track of what's been fetched/what hasn't so we can pass those props off to the component. |
That's exactly the approach that Marty takes, though, and I think it's really valuable - it cuts down on a lot of boilerplate and makes the relevant async fetching code much easier to write. You don't need to explicitly track completed fetches, because those should be handled via the For the average use case where you want to fetch the data once and keep the data around, it's really nice and expressive. I think the only additional thing you might want to do is potentially refresh the data from the client-side in certain instances. |
@cpsubrian had an idea to determine what needs to be passed through by looking at propTypes |
I'm going to do my best to prototype something up as a PR to this branch tomorrow just as a PoC. Didn't get a chance to do so tonight. |
Note that you could support |
48b51f4
to
666f8c1
Compare
|
||
@connect({ | ||
propTypes: { | ||
user: React.PropTypes.string.isRequired, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
container component's propTypes
666f8c1
to
784c76f
Compare
ref #400 - a basic prototype of the resolver/transmit-style API, which I think is actually sorta nice. It also doesn't use React context at all, which is a somewhat orthogonal change. |
784c76f
to
57845d4
Compare
) | ||
} | ||
|
||
if (info.time > info.timeout) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably not the ideal way to handle timeouts - seems like it'd be better to Promise.race
for the timeout, or have a flag on the DispatchBuffer
that gets triggered by the timeout firing; otherwise you end up waiting unnecessarily long.
No description provided.