-
Notifications
You must be signed in to change notification settings - Fork 46.8k
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
Add ReactDataTracker to Addons #3920
Conversation
why not to move import { observeWrite, observeRead} from 'ReactDataTracker';
class Person {
constructor(name) {
this.setName(name);
}
setName(name) {
this.name = name;
observeWrite(this);
}
getName() {
observeRead(this);
return this.name;
}
} |
also possibly it'll be better to take |
@chicoxyzzy Good question. The answer is, you can import only the ReactDataTracker without the whole ReactWithAddons! The methods you will want to look at are The implementation of Re: core-js polyfill, good advice. I'll look into it and may update the diff. We may end up scrapping the polyfill altogether in favor of expando props, so I'll sync with Sebastian about this before churning the diff too much. |
@jimfb I like it. 👍 What would happen if during rendering a non-component uses a getter? For example if a component would ask for a greeting: class Person {
constructor(name) {
this.name(first);
}
setName(name) {
this.name = name;
React.addons.observeWrite(this);
}
getName() {
React.addons.observeRead(this);
return this.name;
}
getGreeting() {
React.addons.observeRead(this);
return 'Hello ' + this.getName();
}
} |
@cody Great question! The answer is that it does the expected/desired thing. Multiple invocations of So you can have your component render function call a helper function like this:
React will still detect the dependency on This means that in your getGreeting() function, you don't need to call |
@jimfb Okay, you have convinced me. I had misunderstood something. 😃 |
Some context would help me understand this better. How is this API expected to be used in real apps? Is Would this API be used by entities like Flux stores? Is raw performance the goal of such a low level API? I'd expect it to be hard to track If I think elaborating on this:
would be very helpful. |
var person = new Person("jimfb"); | ||
React.render(<PersonView person={person} />, container); | ||
person.setName("Jim"); | ||
expect(container.children[0].innerHTML).toBe('Jim'); |
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.
Maybe add additional expect for jimfb
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.
Yep, good idea, will do!
Yes, Yes, the The ability to transparently/seamlessly change the granularity is one of the advantages of this API. Flux store implementors can observe reads/writes at any granularity. As long as they are consistent with regards to where they fire reads and writes, the implementor can choose to fire them at the store level, at the 'root lookup node' of some immutable data, or at an individual element level. They can change their decision at any time without affecting the implementation of components that depend on those stores. This plays nicely with immutable data and allows you to observe anything that is mutable. Datastore implementors can change the mutable/immutable boundaries without changing their public/component API. This API is intended to be easy enough that a user could use it by hand, but the long-term hope is that they would use a data library (analogous to an immutable data library like immurablejs), or that they would use a javascript transform to enhance their datastore code. Internally, we have a transform demo that finds all classes which extend a particular flag class, and automatically adds the calls to startRead, endRead, startWrite, and endWrite. Using the transform makes your code ultra-simple, but is also a little magical, so we are unopinionated and users should do what makes sense for them. It is similar to JSX, in that you get some fancy magic if you use a transform, but you shouldn't feel obligated to use a transform. |
@jimfb I have another question. If I understand it right, then the ReactDataTracker will add overhead to every render and unmount of all components, no matter whether they use this feature. What is the cost for that? |
Some extra context: this is analagous to how Relay works. GraphQL and Relay represent application data as a cyclical graph, because objects in a system are typically interconnected (ex: a user can be the author, liker, and commentator of the same story). But you can't directly represent a graph as a persistent data structure: a single change invalidates the whole graph and little memory can be shared. If nothing is shared between the old and new version of data, there is no way to know what actually changed and what didn't. By extension, there's no way to write an efficient
At first glance it might appear that The alternative is to record the list of record IDs that were read during the course of rendering each component, set up a listener at each component for any changes to its set of read IDs, and update the component when the data for those IDs changes (eg via That's (in part) what this proposal is about.
|
cc @gaearon ^^ |
@cody No, if you don't use this feature, there is no overhead. Certainly nothing that could show up in any measurement. |
} | ||
|
||
for (var componentIndex = 0; componentIndex < componentsToNotify.length; componentIndex++) { | ||
componentsToNotify[componentIndex].setState({}); |
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.
Maybe use ReactUpdateQueue.enqueueForceUpdate(this)
? This seems like it wouldn't work with components with strict shouldComponentUpdate
, and will throw on non-Component-descending components.
@josephsavona Thanks for sharing these details, this proposal makes way more sense to me now. |
@josephsavona This is interesting, much respect for hashing these ideas out in the open, it's really valuable. :) I'm surprised that Relay, even though it works with a full graph structure itself, ends up feeding a cyclical structure to components. I thought one of the primary motivations was to make explicit in the component precisely what data should be side-loaded. In this example, why would you be traversing back from a friend like 'Jim' to the current user 'Joe'? I understand the general utility of a cyclical graph, but expected that with being able to more precisely specify the needs of a component, traversing the graph in multiple directions, beyond just top-down from the root node, would have been irrelevant. Also, if Relay feeds data to components as a cyclical graph, doesn't that also mean it's not serializable without a transformation first? This seems like it would be undesirable, but if so I trust there are good reasons, so just curious to learn more. :) In general, this seems somewhat different than the overall design of React, where shouldComponentUpdate is an optimization hook. Has performance been enough of concerns on low-end devices that approaches to fine-grained change tracking like this become appealing? |
@kevinrobinson Relay can flatten the graph because React components will ultimately form a DAG, but then the problem becomes: "how do you know which DAG nodes to update when the source data changes?". It turns out to be the exact same problem as knowing which components to update. Also, in the more general case, you can't flatten the data ahead of time unless you know how deep the component hierarchy is going to look (something that is impossible to know for a general React app). Regarding serialization of cyclic graphs: Json does not directly support cyclic graphs, but it is possible to implement your serialization on top of json, or using your own protocol/format. Either way, cyclic data is actually pretty common in large applications. Performance is always a consideration. For instance, internally, we are working on an animation API that allows React to render super smooth animations. This is not possible without super high-performance bindings. But having said that, performance is not the primary motivation for introducing this API; please take a look at the related issues (#3858 and #3398) for background/motivation information. |
@jimfb Thanks for the thoughtful and precise response!
This is the bit that's most confusing to me. I thought each component gave a GraphQL expression to Relay to communicate exactly this information? Here's a bit of my current understanding, since I'm not 100% sure I'm following so maybe this will help check. :) I'm picturing Relay has an internal data structure to represent the cyclical graph. I'm assuming it's pure data that have keys/ids that describe circular references. During From the original tree of GraphQL queries, we know which components will need to be updated based on which ones contain nodes or edges in the merged GraphQL query to the server. Is what you're saying that the operations on these graph structures is too expensive? Makes sense to look at this for animations 👍, although for the general use cases described in the other threads, this seems like a more invasive change. |
I only ask so many questions because I really like the ReactDataTracker. Measuring performance is hard, so this might be completely wrong. Here 10k components that don't use ReactDataTracker get rendered with 60fps: var comp = [];
for (var i = 0; i < 10000; i++) {
comp.push({});
}
var start = Date.now();
for (var fps = 0; fps < 60; fps++) {
for (var i = 0; i < comp.length; i++) {
ReactDataTracker.startRender(comp[i]);
ReactDataTracker.endRender(comp[i]);
}
}
var end = Date.now();
console.log(end - start); With iojs (v1.3.0) it is over 100ms on my computer. |
@kevinrobinson Wow, you've really been following along and keeping up with all the technologies! This is a great discussion! You are correct that each component does give (via GraphQL) this exact information to Relay, but incorrect in your assertion that this happens at render; it actually happens statically before the first render, thus allowing Relay to pre-fetch all the information that the component could ever need. As a result, some perfectly legitimate apps can't be described using Relay (eg. a recursive TreeView, where each node is a person and the nodes can be expanded to see the person's friends). Our goal with this API is to solve the Relay use case, but also to provide a more general solution (eg. allow for apps like a recursive TreeView). This is what I was implying when I said that you would need to know "how deep the component hierarchy is going to look". One could argue that this limitation could be overcome by doing as you originally suggested, computing the query at render time. And this is true, but... suppose you did generate an immutable DAG representing your data as needed by a component. Now suppose your underlying data changes, and you need to provide a new DAG; how do you update the DAG? You want to re-use as many DAG nodes as possible, in order to preserve triple equals equality and thus a fast re-render, so you can't just re-generate the DAG. What you really need to do is track which parts of the DAG need to be "fixed" as a result of the data change... but... that's just as hard as figuring out which components need to be updated. In fact, it's actually the same problem, wrapped in a different box. Rather than have each user of React re-implement the same diffing logic, we can use this new API proposal to allow React to figure it out for you! (As an added bonus, by having React do the diffing at the component level, you can automatically avoid re-rendering all the parents of the modified nodes). @cody Yes, the final implementation will change substantially before this is ready to be merged. The source code of this PR is mostly for API demo purposes. The comment in ReactDataTracker (right above the first usage of the Map polyfill) explicitly calls out the perf ramifications of using Map (see the first bullet point in that comment). A final implementation will not have a Map polyfill in the hot codepath of render for components not using this API. I can assure you that the performance impact of this API for components not using the API will be asymptotically close to zero before this code ever ships/merges. |
@jimfb - I couldn't have explained it better myself! @kevinrobinson thanks for the great questions! @jimfb covered just about everything, but a couple extra notes: My comment above was just to illustrate the problem - ultimately, in a cyclical graph when one thing changes, everything changes - and it isn't meant to describe how Relay actually stores data or provides it to components. One of the design goals with Relay is to make it so that applications do not have to worry about the data representation or efficient updates; we push this complex work into the framework. The internal data representation is abstracted away (so that we're free to store data however is most efficient) and the data provided to components is opaque: we guarantee that it will match the shape of the query fragment. Whether we provide a circular data structure or a DAG or something else doesn't matter to the application because the contract is the query format. |
@jimfb @josephsavona awesome explanations, thank you!
This is more about Relay in general, but is still a bit confusing to me - how can this approach work with any branching code in the
As for this particular API, I'm still not entirely following why issues like this are fundamental problems. Maybe an example use case of a component/query that's not top-down and requires the cyclical structure would help? Reading code with actual data structures seems like it'd be the most useful, so I'll also just be curious to follow along and see where you all end up going here. Thanks! :) |
For other interested folks, @leebyron described a bit of this in https://www.codementor.io/ama/1237952640/facebook-react-contributer-lee-byron, particularly here: https://www.codementor.io/ama/questions/8356498072/how-do-deferred-graphql-queries-work |
This of course depends on how you represent the graph. If you represent it as cyclical direct pointers then you're absolutely correct. If you represent it as an adjacency list then you can represent it as a DAG thus immutably and dramatically simplify the "what changed" story. If you use an adjacency list then the "pointers" to other objects just become keys to the top of the graph again. e.g. if you change your profile picture the link "Lee's friend is Joe" will remain the same, so the "Lee" object will be the same post update, but "Joe" will become a new value with the new profile picture applied. Now, when you use an adjacency list like this, any React component that "walks an edge" needs to subscribe to updates from the data store as each node in the graph ("Lee" and "Joe" in this example) is an independent path into the data store DAG which can change independently. This could create a less good API though. I imagine that you might want a getter function for "getFriend" so instead of doing However @jimfb I have one major point of critique for this pull request as it stands: I want to be able to implement this completely in library space - what I would like to do to fulfill the API I described above is pretty different from what you've done in this PR. However since you're baking this PR into the React Core, I won't have the ability to do that. I believe the right way to add the right pieces to React is to add an API to query for the currently rendering component (to query when "reading") and subscribe to unmounts (to clean up). |
Also check out https://github.com/omcljs/om/blob/master/src/om/core.cljs |
@swannodette should help make sure this change makes om less custom since its really similar to ref cursors. |
Right so I've implemented the general concept that this PR proposes not one but two times now :) The first time with ref cursors and I'm doing so again with Om Next sans cursors. Each time I have done this I've had to supply custom base components (that every Om users must use/extend) that we watch for mount/unmount so we can record what data path they are associated with so that we can perform precise updates later. React provides no reasonable hooks so we've had to go our own way each time. I have no specific comments about the specific API design proposed, but I agree with @leebyron's assessment that any such facilities should be exposed so that this feature and similar features can be written purely as libraries. I believe this is an area of React that warrants much broader experimentation and innovation and it's not going to happen via private APIs. |
This is pretty much how I'm solving nested entities with normalizr — instead of reference cycles, I always use IDs and have entity bags appear on the top. It does require additional subscriptions though. |
@leebyron If I understand correctly, this sounds similar to what I've implemented with the "custom classes" Immutable fork. I extend to keep a reference to the "root store" (wrapper for an Immutable.Map) in the inheriting
In my case, the subscription happens in getInitialState() {
return {
// subscribes: ['totals', job.id] -> Immutable.TotalsMap
totals: this.currentJob().totals(),
// subscribes: ['report', job.id, 'element_costs'] -> Immutable.ReportMap
report: this.currentJob().report().observing('element_costs')
}
} This has worked excellently, but this use of Also, from @sebmarkbage in #3398:
IIRC a more generic hook here (after Perhaps |
@kevinrobinson In Relay, a component's data needs are specified as a query, and the query may specify deep dependencies based on data (example query: "for each news feed item, get all comments, for each comment, get the author, for each author, get the name and profile image"). Obviously you need to know who commented in order to know which profile images to fetch, but that's why the GraphQL queries are so expressive. And yes, a component needs to fetch all the data for any branch that it will take in render; the programmer needs to figure that out in advance (it is not automatic). That was one of the design decisions made in Relay. But let's move this tangential discussion to another thread; the details of Relay aren't required for an understanding of this API. The key insight is that when underlying data changes, determining the ramifications of those changes on the component tree is non-trivial (and that's what this API aims to fix). @leebyron Absolutely, great points! Regarding API: In general, I totally agree, it's great to be able to push code into user/library space whenever possible. In this case, however, we thought it through, and there are several reasons we would want this in the core instead of user/library land. One of which is that by managing subscriptions internally, the "side effects" are an internal implementation detail (we keep our options open to refactor later and make the entire core immutable and render pure). Another is that there are optimizations we can build into the core, which are not possible if this is implemented in user-land. Jordan made an internal fb-only post a few hours ago (about timer propagation), which is worth reading. Another is that we might want to use this functionality internally (for things like context? pruning) which would require that this functionality be in the core anyway. In summary, we want to push things into user/library land whenever possible, but it was a conscious design decision to expose this higher level API instead. Feel free to grab me and Sebastian if you'd like to discuss more. @swannodette points out that he has written this same logic multiple times. Internally to Facebook, we've also seen this exact logic implemented several times (Animations, Relay, etc). That's exactly the motivation behind this API (it is a pattern we see people implementing over and over again, and it's difficult to get it right/optimized). |
@jimfb to be clear the API so far described is completely useless for Om. We will continue to do our own thing. |
I'm out this week, but I'd love to meet up and discuss more with you guys. Concretely, what I want from React Core in this instance are the low level API hooks which would allow ReactDataTracker in a generic form. As presently implemented, it would require mutable data which I obviously am concerned about and won't help me build the kinds of APIs I want to support for React + Immutable.js and I believe also will not help @swannodette simplify the development of om for the same reasons. Specifically the |
@leebyron Yeah, I agree, that's ideal, just don't know how to do it without disrupting our other/future plans. With regards to the dependency on long-lived object references... this is easily solved by emitting reads/writes on whomever manages the data (eg. datastore) instead of the actual immutable data objects themselves. This shouldn't be incompatible with immutable in any way. But yeah, let's chat more when you get back! @swannodette True, any system which tracks dependencies outside of React wouldn't benefit from having React track dependencies (sorta self-evident). In that sense, this API is no better or worse than the alternative |
@jimfb Thanks for explaining the Relay bits further, and apologies for the tangential questions. What @leebyron was describing is how I figured Relay actually worked, and why I think I was misunderstanding why "determining the ramifications of those changes on the component tree is non-trivial" and asking about the internals of how you're representing the query and data. :) If the problem here is that Relay is feeding React components large object structures with getter methods, and it is difficult to write an efficient To propose a naive solution for the For the more typical case of side-loading from an external store, setting the component's state, and implementing
@leeb This sounds like an interesting avenue - this and what @elierotenberg suggested in #3398 (comment) sounds like the most appealing direction for encouraging community experimentation. Or @jimfb if this has been a recurring problem inside FB perhaps sharing more stories there would help others understand the motivation. Another tactical suggestion would be to take what you have here and do the same thing with a fn wrapping the component or just the render method. trackingRender = function(render) {
var tracker = new ReactDataTracker(render);
tracker._cacheValid = false;
return function() {
console.log('before render');
var renderedComponent = tracker.read();
console.log('after render');
return renderedComponent;
};
};
// in your component, wrap `render` or wrap the whole class and inject this tracking code.
render: trackingRender(function() {
// ...
} If the desire is to make doing this more first-class, perhaps the more general change in the same spirit as this PR that would allow other libraries to experiment would be to support wrapping all |
@kevinrobinson Yep, absolutely. That's a completely valid alternative API (semantic derivative of what Lee was proposing). The issue is that By having React control the subscriptions, all side effects are internal to React (therefore, React can undo them or choose not to apply them, or whatever). |
Hi @jimfb, As far as I can deduct from this threat, I think we've implemented something quite close to intentions of your PR in the MOBservable library. It allows react components to sideways load data if annotated with a simple mixin (or decorator). Similar to the PR, and similar to Meteor Tracker, it tracks reads during the render phase, but a lot more implicit; especially, you don't need to explicitly call a tracker from inside your data structure, as long as you annotate your data objects. Besides plain values MOBservable also supports observing complete arrays and objects and updates are always processed atomically (without needing flushes) to avoid unnecessary re-renders. Since, like Meteor Tracker, it doesn't rely on an API exposed by React, it IMOHO more cleanly separates responsibilities, as these reactive data structures are useful independently of the fact whether they are used in a React component. Anyway, if that sounds interesting, here is a diff from rewriting TodoMVC to use reactive structures instead of immutables, and here is an in-depth motivation including a performance analysis. I referred to this lib in #3398 as well so you might have already seen it. I don't want to be spamming, but I would argue that it is best for react to leave as much problems in userspace as possible, to keep the learning curve small because I think that that is exactly what makes React so awesome (compared to Angular for example ;-)) |
@mweststrate Yep, saw it, and read through the docs. Although I haven't played with MOBservable, I agree it looks pretty similar to this proposal for React. I would even go so far as to suggest that the implementation is likely be better than this proof-of-concept implementation, which I wrote up in a few hours (we would fix our implementation before a final release). Conceptually, I agree with you 100%. However, there are reasons such an API really needs to be supported by the React core in order to work efficiently/correctly. The problem is: either you need to do subscriptions in advance or lazily. Let's explore both options... If you do subscriptions in advance: you necessarily must oversubscribe to your data, since you need to subscribe to anything you could ever need to read in render. You often get into situations where virtually every component on the page is subscribing to almost everything, and now you're back to where you started (slow performance). If you do subscriptions lazily: you can solve the oversubscription problem, but now you need to change your subscriptions on the fly based on which mobservables your component is reading during render. This works (it's the solution used in this RFC/PR) but it means one of two things:
MOBservables is a super cool proof of concept, but it is necessarily insufficient because it is only possible/legal to do dynamic read subscriptions if you are the React core. I encourage you to continue iterating and exploring the area, because we pull a lot of our inspiration from the community's work. Having said that, if this is a style you'd like to see us support, let us know so we can prioritize it! |
Hi @jimfb, Thanks for the explanation. I think the only way for subscriptions to perform is to subscribe lazily indeed. This indeed introduces side-effects. I thought about how this fact could make the React abstractions leaky during the design of MOBservable, but I couldn't come with anything since its basically just a short circuit to To achieve the desired purity I think however that you could slim down your API a bit. Unless you want to build a complete observable framework on top of React, the API bridges should be as small as possible. In think the pseudo code below could make it possible for external libs to be less aware of the fact that React relies on their observables. I think the following API might fit in more cleanly (but its just a thought experiment) // register a observable tracker
ReactDataTracker.registerTracker(tracker:ITracker)
interface ITracker {
startRead();
endRead():IObservable[];
}
// pseudo code:
reactComponent.render() {
this.currentSubscriptions.forEach(sub => sub.dispose()); // see note below
ReactDataTracker.tracker.startRead(); // given there is one, or there might even be multiple different trackers in the system
render();
var observables = ReactDataTracker.tracker.endRead();
this.currentSubscriptions = observables.map(o => o.observe(this.forceUpdate.bind(this)));
} I think this API would be easier to implement by libs. It uses a general Observable API after all, without needing different observable implementations to be concerned about registering writes. The reads are easier to register as libraries like MOBservable and Tracker already have a central place where reads are tracked. With the proposals above the libraries don't have to support a second way to register reads and writes but can keep their existing subscription management system. Although not all tracking concerns are pulled into React, React itself is now responsible for managing subscriptions. I think there is a similarity compared to the DOM; while not all side-effects happen inside React, but they are at least controlled by React. On a side note: there is a lot of performance to gain by not disposing subscriptions before rendering; it is cheaper to diff the list of observables first, since the list of observed things stays usually pretty constant over time. MOBservable gained significantly from such an optimization. |
@jimfb We've talk about an issue internally about how could you use this API to stop listening to a data source based on its value? What's the alternative solution when you can be sure you're not going to need the value anymore? E.g. the timer / alarm scenario. I guess you can never be sure since you should be able to do time travel etc. unless something in state changes. |
@sebmarkbage Yeah, I think that you solve that the same way you would solve this if it were standard javascript and you wanted to stop reading from a stream. You need to have some state variable to indicate when you're done. For a timer/alarm/server-subscription, which requires an imperative unsubscribe, I think we'd just use a wrapper like in #3858 to be notified of the unsubscription and imperatively dispose of the subscription. |
It appears that there was no consensus on this. I’m closing as we are trying to avoid stale pull requests and be more decisive, and discuss in the issues instead. Please feel free to continue the discussion in #3858. |
Internally, there has been serious discussion about exposing an API that allows data reads and writes to be tracked, as an alternative to the Observe API proposed a couple months back. This new API allows React to automatically track a component's dependencies, thereby allowing React to efficiently re-render a component when one of the dependencies changes. Usage is a little more intuitive (and a little more flexible) than the Observable API (also more intuitive if you are using Observables, see #3858, so it's a win-win), but since the data bindings are tracked automatically, it's a little less explicit. For anyone following along at home, take a look at the unit test in this commit for example usage.
Posting this commit/PR mostly to continue/inform the discussion. We aren't ready to merge this quite yet, as the final disposition of this API proposal is still undecided.
Fixes: #3858