-
-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
Proposal: <Routes dataSource> #405
Comments
I am kind of refactoring to this in my My only concern is that its a lot of convention and indirection, which I hate documenting, and is generally rejected in react. Would be cool if we could build this on top of something like Its the |
What's the point of dataTypes? Can't those just be propTypes? var BlogPost = React.createClass({
statics: {
getDataKeys: function (params, query) {
return {
currentUser: 'users/current',
author: 'posts/' + params.postID + '/author',
post: 'posts/' + params.postID
};
}
},
propTypes: {
currentUser: React.PropTypes.instanceOf(User),
author: React.PropTypes.instanceOf(User),
post: React.PropTypes.instanceOf(Post)
}
}); |
I don't understand the word "indirection". People use it all the time, but I have no idea what they're talking about. Can you explain?
|
|
No, because they're not props. |
Thank you for quoting Wikipedia. Trust me, I've read that article and I still don't get it. |
I also don't like that I may have to now parse this key to perform the operation in my dataSource, the router already parsed it for me.
They are eventually. |
Not necessarily. They only become props if you use them as props. In my |
Maybe we could send more than just the key to data source, function dataSourceHandler(payload) {
payload.routeName
payload.params
payload.query
payload.keys
} |
sometimes the route name will matter, other times it won't at all, or even send the route handler so people can then put static methods on them to keep the code in the same place as getRouteProps |
yeah send payload.route instead of routeName, then you have a bunch of information |
You guys make me laugh. |
@mjackson I like this, I'll screw around with some fake code now that I've got my head wrapped around the constraints of building a server and client rendered app. I'm sure I'll have some questions at the end of the day. Right now I have foggy thoughts and concerns about how the lookup/caching lifecycle works. I want people to have their own data layers that this just plugs into minimally and explicitly. |
Totally. I don't want to get into managing any of that explicitly. I was thinking we would call the |
@rpflorence The whole idea of keys may be too tailored to my use case. I'm using a key/value store as my backend, so it appeals to me. But I do find the idea of being able to refer to each piece of data in your system by some key/URL very appealing. |
@spicyj Is there any way we can hook into the |
Yeah, it works great for the server-render, and then the initial client render. After that you'll need subscriptions, not sure if we should handle this, or if we just say to subscribe in Why do |
No. In a few releases there'll just be plain ES6 classes and it'll look something like facebook/react#1380, so you can do whatever's possible there. What's wrong with |
@rpflorence @spicyj Nothing's wrong with |
I personally re-use my keys for subscribing to data. I can do a one-time fetch or subscribe using the same key. |
What if we forked the dependencies (like Stores) accordingly to the environment? /* on client-side */
var routes = require('./routes');
// Inject dependencies throught context
React.renderComponent(React.withContext({
MessagesStore: new ClientMessagesStore(),
ThreadStore: new ClientThreadStore()
}, function() {
return routes;
}),document.body); /* on server-side */
// Inject dependencies throught context
renderRoutesToString(React.withContext({
MessagesStore: new ServerMessagesStore(),
ThreadStore: new ServerThreadStore()
}, function() {
return routes;
}), req.path, function() { /* ... */ }); If there were anyway to inject the context on static methods like the one bellow, we would solve this gracefully React.createClass({
statics: {
willTransitionTo: function(transition) {
transition.wait( this.context.MessagesStore.getAllMessages());
}
}
}); |
I did a bunch of work on this idea in the data-types branch, but at this point I'm thinking that |
I haven't fully crystalized my thinking about this yet, but I had a great conversation with @petehunt last week where he told me about a few ideas he had about data fetching. I've had a few thoughts since, but hopefully I haven't strayed too far from the meat of it. Hopefully Pete can correct me where I messed up.
The Problem
We need a way to get data into the views. The router is fairly well-suited to solve this problem because it decides what components render on the page given the current URL.
A Possible Solution
There are a few parts to this solution:
First, "route handler" components would have a way to declare the types of data they are interested in using some unique key. For example, a route handler could indicate it needs the
currentUser
orpost
variable. Then, it would have another method that returns a hash of these keys to strings (think URLs) that uniquely identify those pieces of data. This could look like:Secondly, The
<Routes>
object would have adataSource
. ThedataSource
is responsible for fetching data based on the (de-duped) set of keys generated by the currently active routes. It could be as simple as a function that takes an array of the keys and returns an array of values/promises.Lastly, any component that needs some data simply mixes in
Router.Data
and declares thedataTypes
it needs:Benefits
The only real advantage I can think of for this approach over
getHandlerProps
is that branching for data fetching happens in yourdataSource
instead of your route handlers.There is also the fact that you don't have to pass
props
around, but you still get the benefits of declaring the types of data you need (similar topropTypes
) so it shouldn't be too hard to refactor stuff.Thoughts?
Edit: Every time we talk about data loading the thread gets a hundred comments. If you're going to comment here, please keep your comments specific to this proposal. If you have another idea, spin up a separate issue. Thanks!
The text was updated successfully, but these errors were encountered: