facebook / react

The library for web and native user interfaces.
https://react.dev
MIT License
227.32k stars 46.37k forks source link

Implement Sideways Data Loading #3398

Closed sebmarkbage closed 6 years ago

sebmarkbage commented 9 years ago

This is a first-class API for sideways data loading of stateless (although potentially memoized) data from a global store/network/resource, potentially using props/state as input.

type RecordOfObservables = { [key:string]: Observable<mixed> };

class Foo {

  observe(): RecordOfObservables {
    return {
      myContent: xhr(this.props.url)
    };
  }

  render() {
    var myContent : ?string = this.data.myContent;
    return <div>{myContent}</div>;
  }

}

observe() executes after componentWillMount/componentWillUpdate but before render.

For each key/value in the record. Subscribe to the Observable in the value.

subscription = observable.subscribe({ onNext: handleNext });

We allow onNext to be synchronously invoked from subscribe. If it is, we set:

this.data[key] = nextValue;

Otherwise we leave it as undefined for the initial render. (Maybe we set it to null?)

Then render proceeds as usual.

Every time onNext gets invoked, we schedule a new "this.data[key]" which effectively triggers a forcedUpdate on this component. If this is the only change, then observe is not reexecuted (componentWillUpdate -> render -> componentDidUpdate).

If props / state changed (i.e. an update from recieveProps or setState), then observe() is reexecuted (during reconciliation).

At this point we loop over the new record, and subscribe to all the new Observables.

After that, unsubscribe to the previous Observables.

subscription.dispose();

This ordering is important since it allows the provider of data to do reference counting of their cache. I.e. I can cache data for as long as nobody listens to it. If I unsubscribed immediately, then the reference count would go down to zero before I subscribe to the same data again.

When a component is unmounted, we automatically unsubscribe from all the active subscriptions.

If the new subscription didn't immediately call onNext, then we will keep using the previous value.

So if my this.props.url from my example changes, and I'm subscribing to a new URL, myContent will keep showing the content of the previous url until the next url has fully loaded.

This has the same semantics as the <img /> tag. We've seen that, while this can be confusing and lead to inconsistencies it is a fairly sane default, and it is easier to make it show a spinner than it would be to have the opposite default.

Best practice might be to immediately send a "null" value if you don't have the data cached. Another alternative is for an Observable to provide both the URL (or ID) and the content in the result.

class Foo {

  observe() {
    return {
      user: loadUser(this.props.userID)
    };
  }

  render() {
    if (this.data.user.id !== this.props.userID) {
      // Ensure that we never show inconsistent userID / user.name combinations.
      return <Spinner />;
    }
    return <div>Hello, {this.data.user.name} [{this.props.userID}]!</div>;
  }

}

We should use the RxJS contract of Observable since that is more in common use and allows synchronous execution, but once @jhusain's proposal is in more common use, we'll switch to that contract instead.

var subscription = observable.subscribe({ onNext, onError, onCompleted });
subscription.dispose();

We can add more life-cycle hooks that respond to these events if necessary.

Note: This concept allows sideways data to behave like "behaviors" - just like props. This means that we don't have to overload the notion state for these things. It allows for optimizations such as throwing away the data only to resubscribe later. It is restorable.

glenjamin commented 9 years ago

It seems to me like the last few comments are saying that the smallest extension point is actually "connect" and "disconnect" lifecycle hooks - which make it easy to attach async data to a component and manage those subscriptions?

Observables could then be fairly trivially built upon that (in or outside core) - but exposing those lifecyle points has a broader appeal?

Is that a reasonable summary?

fisherwebdev commented 9 years ago

I am also not yet convinced that this needs to be solved in React itself. I am much more inclined to think that React should provide any necessary hooks to build this functionality on top of React.

But for a moment, let's say we're going with observe(). A few thoughts:

We have this.props, this.state, this.context and now this.data, all as potential sources of new data in render(). This seems excessive to me. Is the idea to separate application state from component state? This might clarify and separate some issues around state, but I feel like the cost of introducing a new input may not outweigh the gains. If we want this.state to be focused solely on component state, why not let the fields in this.data add to this.props or this.context instead?

The name this.data is too generic. Props are data, state is data, and any local variable is data. The name adds no meaning and muddles existing meanings. I would much prefer this.observed or any other name that would actually mean something. So +1 to @matthewwithanm's comment:

might there be a better name than data? For example, observed would more clearly associate it with the method.

If we let observe() execute on the server, we need some kind of hook that will clean up any memory leaks this might cause, because unmounting will never happen.

If we call observe() again every time props or state change (and context?) then observe must be optimized for performance and ideally can't be made expensive accidentally. It becomes a part of the hot path. I like this from React Nexus:

the next bindings are diffed with the previous bindings; removed bindings are unsubscribed, added bindings are subscribed.

gaearon commented 9 years ago

I've come to believe that state complicates components and I've been trying to use it less in my own components and lift it up instead. This is why, in addition to concerns @fisherwebdev raised (which I agree with now), I'm not convinced that letting observe depend on state is a good idea. State depends on props, observed depends on state and props.. Isn't it too complicated? I'd rather have observe(props).

andrewimm commented 9 years ago

I'm also coming to realize that observe should not be dependent on state, mostly because it doesn't need to. As @gaearon points out, it's easy enough to hoist the state to one level above, which just feels cleaner after separating concerns. When observe can potentially depend on state, the logic around handling updates within the component grows significantly more complex; when it only depends on props, you can have fork-less handlers in componentDidMount/componentWillReceiveProps. Less code in the critical path translates to a simpler rendering cycle, and it also reduces the number of unintended updates that re-trigger subscriptions.

jeffbski commented 9 years ago

+1 for observe(props)

The less we deal with state the better IMO.

nmn commented 9 years ago

I'm of the opinion, that as a library, React should try to be as flexible as possible. I agree that it's not a good idea for observe to depend on state. Yes it can be complicated. Yes, the best practice should be to not depend on state. But that is a choice that users of React should be allowed to make. I would recommend, leaving the current API unchanged (other than any possible hooks to add more flexibility, to make it work with more than just observables for example), and proving documentation that explains that using state in the observe method is not recommended.

nstadigs commented 9 years ago

I believe that everyone wants to do the right thing, and that we want to be pointed in the right direction. Having observe not accepting state makes that easier for the user than to accidentally find something like "this is an antipattern" in the docs.

ianobermiller commented 9 years ago

EDIT: sorry, just realized I was looking for flatMap. I confused myself because I was thinking it would flatten the array of messages, but it is operating at a higher level (the messages observable).

Would the proposed API handle the case where the result of one data field is dependent on the result of another? I.e. you map the first result and return an observable:

observe(props, context) {
  if (!props.params.threadID) {
    return {};
  }

  const observeThread = ThreadStore.observeGetByID(
    {id: props.params.threadID}
  );
  return {
    thread: observeThread,
    messages: observeThread.map(thread => {
      return MessageStore.observeGetByIDs({ids: thread.messageIDs});
    })
  };
}

I'm pretty new to observables in general, so I may be going about this completely wrong. In promise-land this is extremely simple, since returning a promise from then will cause subsequent thens to be based off that promise.

sebmarkbage commented 9 years ago

I don't understand the comments that it shouldn't depend on this.state. Encapsulated state surely makes React a lot more complicated but that is also all it is. If there wasn't encapsulated state, we would only need a memoized immediate mode library. If you go all in on "Stores", then yes, you don't need state but that is not what React prescribes that you do.

We have several patterns that requires you to create extra wrappers but for normal usage you shouldn't need to. Other than stores, your observed data always depends on state even if indirectly. I don't think it is bad practice at all to depend on it in observe. E.g.

observe() {
  return { items: Items.getPagedItems({ pageIndex: this.state.currentPage }) };
}

Even if observe didn't depend on state it would still depend on props and context. Even if it didn't depend on context you would still have an indirection that uses context to render a component's props that uses it in observe.

observe would need to get reevaluated every time a render pass comes down because the props could've changed. However, we would definitely diff the resulting observables and not unsubscribe/resubscribe if the same observable is returned. However, we can't do diffing on individual properties (other than with shouldComponentUpdate) so ideal you should implement your own cache using a Map as a power feature. That way you can return the same observable to multiple components in the tree. E.g. multiple components loading the same user. Even if you don't, you just recreate the Observable and eventually hit the bottom cache. This is just how React reconciliation works anyway. It's not that slow.

This observe hook isn't designed to make React completely reactive in the sense that state is captured in Observables. It is currently a key design goal is to avoid trapping state in closures and combinators and instead have a nice clean separate state-tree that can be frozen and revived, and potentially shared across workers.

Which brings me to my final point...

We certainly don't need to add this to the core library. The original "public" interface was mountComponent/receiveComponent and you could build your whole composite component system on top of it. However, not many people used that it is much more powerful to raise the abstraction bar since we can now build other things that are enabled by a higher abstraction bar. Such as component-wide optimizations.

React's primary purpose is to create a contract between components so that different abstractions in the ecosystem can co-exist. An important part of that role is to raise the level of abstraction for common concepts so that we can enable new cross-component features. E.g. saving all the state of a subtree, then revive the subtree. Or possibly including automatic unmounting on the server or changing the timing aspects of reconciliation on the server.

It is also important to provide some batteries included to make all this palatable and uniform.

It is important to realize that micro-modularization (like adding a new life-cycle hook) isn't strictly a pure win over building it into the framework. It also means that it is no longer possible to reason about system wide abstractions.

gaearon commented 9 years ago

I wish something like this was in the docs as “philosophy / design goals / non-goals”.

acdlite commented 9 years ago

React's primary purpose is to create a contract between components so that different abstractions in the ecosystem can co-exist. An important part of that role is to raise the level of abstraction for common concepts so that we can enable new cross-component features.

Love this. I agree with @gaearon that it'd be nice if this were in some sort of doc.

jquense commented 9 years ago

We certainly don't need to add this to the core library.... However, not many people used that it is much more powerful to raise the abstraction bar since we can now build other things that are enabled by a higher abstraction bar. Such as component-wide optimizations.

I feel reticence (for me at least) is not in adding another API, but adding one that depends on a non-language (still being defined) construct to work. This could totally work out fine, but I worry about the issues Promise libraries face where no promise library (even spec complaint ones) can trust each other, which leads to unnecessary wrapping and defensive work to make sure they resolve correctly, which limits optimization opportunities. Or worse yet you get stuck like jQuery with a broken implementation that can never change.

sebmarkbage commented 9 years ago

@jquense I completely agree. I wanted to add this hook a long time ago. (Original experiment: https://github.com/reactjs/react-page/commit/082a049d2a13b14199a13394dfb1cb8362c0768a )

The hesitation two years ago was that it was still too far away from standardization. I wanted a standard protocol before we added it too the core.

I think we're getting to a point where a lot of frameworks agree on the need for something like Observables and standardization is reaching a point where a palatable API has been proposed. I'm sure we'll need to end up tweaking it a bit but as long as the high level architecture works it should be swappable and eventually converge.

I think what happened with Promises is that the API and debugging story was severely lacking in certain areas that Observables doesn't suffer from. It is a more complete story out-of-the-box where as Promises had to standardize a minimal incomplete solution.

gaearon commented 9 years ago

The only difference in opinion re: Observables I've observed (couldn't resist, sorry) is Zalgo potential. Whether or not Observables may push value synchronously in response to a subscription. Some folks seem against it, but React's use of Observables will depend on this as far as I understand. Can you comment on that?

Generally I don't find Zalgo to be a problem with Observables because consumer is always in control and may opt for always-async with something like observeOn.

nmn commented 9 years ago

Glad that there's finally some consensus on this. I personally prefer channels over Observables, but if Observables are going to be added to the language, I agree there is no need to wait any longer.

That said, let's make sure that we keep the API open enough to work with non-Observables that conform to the basic API.

jeffbski commented 9 years ago

I don't find Zalgo to be a problem either. However with Observables you can use a scheduler to ensure async if desired, the default scheduler is now async so you can use that as needed.

fisherwebdev commented 9 years ago

@sebmarkbage I think you've addressed most of my concerns and I see now the benefit of adding this to the framework. However, can you comment on this.data -- (1) can/should we fold those fields into props/context/state or (2) can we rename it?

jquense commented 9 years ago

Slightly bikeshed-y but going for it anyway...the Zalgo issue isn't really one about whether it matters in terms of API expectation, it is one of observable interop and ease of implementation. Not having early agreement about Zalgo that has put the Promise library world in the annoying position of having to be super defensive when dealing with Promises from other libs. (my above point repeated below)

...where no promise library (even spec complaint ones) can trust each other, which leads to unnecessary wrapping and defensive work to make sure they resolve correctly

Because early promises didn't all comply with async resolution we are in a position now where even tho it is speced libraries can't assume thenables are trust worthy, killing potential optimizations. This strikes me as particularly relevant here, where React will not be providing an Observable implementation to use (who would want that anyway?), and we are most likely years away from being able to rely solely on a browser provided Observable, so easy library interop is important. Plus to @gaearon's point, if React depends on sync calls and it is spec'ed to always be async that puts us in a jquery like position of being stuck with a rogue implementation.

I completely agree. I wanted to add this hook a long time ago. The hesitation two years ago was that it was still too far away from standardization. I wanted a standard protocol before we added it too the core.

I am glad its being attended too and thought about, that is certainly comforting. :) and in general I think the early adoption of promises has been worth the cons i'm discussing here, so don't take my concern as dislike, or disapprove, I am pretty excited at the prospect of a first class API for this and I also see how Observable is really a good/most reasonable choice here for it.

vladap commented 9 years ago

"We should use the RxJS contract of Observable since that is more in common use and allows synchronous execution, but once @jhusain's proposal is in more common use, we'll switch to that contract instead."

Just to add a little bit more context. There is a Reactive Streams initiative (http://www.reactive-streams.org/) to provide a standard for asynchronous stream processing with non-blocking back pressure. This encompasses efforts aimed at runtime environments (JVM and JavaScript) as well as network protocols.

The current leading implementations are f.e. Akka Streams or RxJava. I don't know if RxJs already comply to the same interface now, the current interface for Subscriber is onSubscribe(Subscription s), onNext(T t), onCompleted(), onError(Throwable t).

Can you shed more light on what is @jhusain's proposal?

I don't know if React should strictly comply to this initiative because if I need I can probably put RxJs (assuming it will comply) in between and adapt to React interface and let more advanced concepts like back-pressure to RxJs (though I would prefer not have to adapt much).

Is there any position or goal regarding this initiative?

Shenlok commented 9 years ago

@vladap I believe this is the mentioned proposal from @jhusain

vladap commented 9 years ago

I have read through @jhusain and I'm not sure about the motivation to possibly move to this spec in the future. Is there any specific advantage?

Reactive-streams spec has larger support and is already on version 1.0. Because RxJava already implements this spec I assume RxJs will follow (but haven't checked).

This blog sums up the interfaces with some examples using Akka streams.

I can see some advantage to have the same interfaces on the backend and the frontend, mainly because I work on both. Possibly it might help to cooperate between backend and frontend groups but on the other hand I assume that websocket or sse are the actual integration points for streaming.

I can't find the implementators list on www.reactive-streams.org right now but the last time I checked it was:

Björn Antonsson – Typesafe Inc. Gavin Bierman – Oracle Inc. Jon Brisbin – Pivotal Software Inc. George Campbell – Netflix, Inc Ben Christensen – Netflix, Inc Mathias Doenitz – spray.io Marius Eriksen – Twitter Inc. Tim Fox – Red Hat Inc. Viktor Klang – Typesafe Inc. Dr. Roland Kuhn – Typesafe Inc. Doug Lea – SUNY Oswego Stephane Maldini – Pivotal Software Inc. Norman Maurer – Red Hat Inc. Erik Meijer – Applied Duality Inc. Todd Montgomery – Kaazing Corp. Patrik Nordwall – Typesafe Inc. Johannes Rudolph – spray.io Endre Varga – Typesafe Inc.

Maybe I go too far here but I believe that the larger context can help in future decisions.

fdecampredon commented 9 years ago

@vladap From what I understand and what I see on the github issues @jhusain is already working with them, so I guess we will not have so much problem. From an interface perspective, from what I also could grasp in different github issues and other spec document, observer will certainly respect the generator interface so something like :

{
  next(value),
  throw(e),
  return(v)
}

Simply implementing a very basic observable with a single 'subscribe' method that respect that interface should be secure for react.

vladap commented 9 years ago

It looks like just a different naming with the same functionality, in that sense it is fine. I would probably prefer the same naming as in the spec but in the end I don't care that much as far as these methods do the same.

I can''t evaluate missing equivalent to onSubscribe(). In the blog I have mentioned author says that it is a key to control back-pressure. I don't know it has other use cases. From this I assume that React doesn't care about controlling back-pressure or there is another strategy for it. It is a complex thing hence I understand it is not React concern.

Do I understand correctly that the strategy is something along the lines - either the app is complex and there can arise back-pressure problems then use something in between to solve it, like RxJS, or you connect React component directly to f.e. websocket and you don't have back-pressure problems because the app is simple and have slow updates.

vladap commented 9 years ago

Where can I find proposed observable interfaces for the future ECMAScript? If there any already.

jhusain commented 9 years ago

The current proposal can be found here:

https://github.com/jhusain/asyncgenerator

JH

On May 7, 2015, at 2:32 AM, vladap notifications@github.com wrote:

Where can I find proposed observable interfaces for the future ECMAScript? If there any already.

— Reply to this email directly or view it on GitHub.

jhusain commented 9 years ago

The Reactive Streams Proposal (RSP) goes further than the TC-39 proposal because it introduces an Observable that handles back pressure. The RSP Observable is optimized for efficiently sending streams across the network, while respecting backpressure. It's based in part on the work done in RxJava, which is a very impressive piece of engineering (full disclosure: it was designed by colleague at Netflix, Ben Christensen).

The primary reason for the decision to standardize the more primitive Observable type is caution. The more primitive Observable is the dual of the ES2015 Iterable contract, which provides us with valuable guarantees that the type is at least as flexible as a type already standardized in ES2015. Furthermore there are a wide variety of use-cases in JS for Observables which do not require backpressure. In the browser the DOM is the most common sink for push streams, and acts effectively like a buffer. Given that the RSP type is more complex, our approach is to standardize the more primitive type first, and then leave room to implement the more advanced type later. Ideally we'd wait until it was validated in user-land.

FYI RxJS currently has no plans to implement RSP Observable.

JH

On May 7, 2015, at 2:30 AM, vladap notifications@github.com wrote:

It looks like just a different naming with the same functionality, in that sense it is fine. I would probably prefer the same naming as in the spec but in the end I don't care that much as far as these methods do the same.

I can''t evaluate missing equivalent to onSubscribe(). In the blog I have mentioned author says that it is a key to control back-pressure. I don't know it has other use cases. From this I assume that React doesn't care about controlling back-pressure or there is another strategy for it. It is a complex thing hence I understand it is not React concern.

Do I understand correctly that the strategy is something along the lines - either the app is complex and there can arise back-pressure problems then use something in between to solve it, like RxJS, or you connect React component directly to f.e. websocket and you don't have back-pressure problems because the app is simple and have slow updates.

— Reply to this email directly or view it on GitHub.

vladap commented 9 years ago

Many thanks for the valuable details. It makes a lot of sense.

amccloud commented 9 years ago

@gaearon I sorta copied you. I wanted to use ParseReact with ES6 classes so I needed to reimplement the mixin's observe api as a higher-order component.

https://gist.github.com/amccloud/d60aa92797b932f72649 (usage below)

gaearon commented 9 years ago

@aaronshaf @gaearon The benefit of making it first class is:

1) It doesn't eat away at the props namespace. E.g. the higher-order component doesn't need to claim a name like data from your props object that can't use for anything else. Chaining multiple higher order components keeps eating up more names and now you have to find a way to keep those names unique. What if you're composing something that might already be composed and now you have a name conflict?

Besides, I think that best-practice for higher-order components should be to avoid changing the contract of the wrapped component. I.e. conceptually it should be the same props in as out. Otherwise it is confusing to use and debug when the consumer supplies a completely different set of props than is received.

Instead of a HOC, it can be a regular component:

import Observe from 'react/addons/Observe';

class Foo {
  render() {
    return (
      <Observe
        render={this.renderData}
        resources={{
          myContent: xhr(this.props.url)
        }} />
    );
  }

  renderData({ myContent }) {
    if (myContent === null) return <div>Loading...</div>;
    return <div>{myContent}</div>;
  }
}

Because you control the function passed as render prop, there is no way for the names to collide. On the other hand, it does not pollute the owner's state.

What am I missing here?

gaearon commented 9 years ago

This could even be less verbose if Observe component just grabbed Observables from its props:

render() {
  return (
    <Observe myContent={Observable.fetch(this.props.url)}
             render={this.renderData} />
  );
}

renderData({ myContent }) {
  if (myContent === null) return <div>Loading...</div>;
  return <div>{myContent}</div>;
}

What's also nice about this is it'll avoid re-subscriptions if shouldComponentUpdate returned false because we won't get into render at all.

gaearon commented 9 years ago

Finally, one could write a decorator for render that wraps it into an Observe component:

@observe(function (props, state, context) {
  myContent: Observable.fetch(props.url)
})
render({ myContent }) {
  if (myContent === null) return <div>Loading...</div>;
  return <div>{myContent}</div>;
}
fdecampredon commented 9 years ago

I would prefer to keep render as a pure rendering function instead of injecting data-fetching logic in it. The initial proposal seems good in my opinion. It's very close of how state works with rx-react and it will allows to separatie state management from data-fetching logic which seems very coherent.

The only thing that bother me is the use of map of observable instead of one observable, because it does not give the user the possibility to choose how observable are composed but this is a minor issue.

gaearon commented 9 years ago

It's not really injecting the data fetching logic, just saves some typing. It will desugar to the version above, which just renders <Observe /> component. It's not unusual to render stateful components, so I don't think it makes render any less pure than it is now.

I tried to combine the best from #3858 and this proposal.

In any HOC approaches, the benefit is explicitness, but the downsides are described by @sebmarkbage: the props names may conflict at some point.

In the current proposal, the benefit is explicitness, but the negative side is the more complicated lifecycle and larger core component API surface.

In #3858, the benefit is colocating "memoized render" dependencies with rendering itself (they're not used anywhere else so it makes sense), but I'm concerned about the “looks sync but is async”, and not understanding how it can work with immutable models if it relies on this so much. It also rubs me wrong in the React-was-easy-to-reason-about way because there's nothing easy about reasoning about manually tracking changes and coupling the data sources to React (or wrapping them to work with React). I understand there exists a pressure to implement something both performant and reducing the boilerplate though.

In my proposal, I'm keeping the colocation and explicitness, but:

kevinrobinson commented 9 years ago

Great discussion and work all around here, much respect. :)

I agree that this doesn't merit making it into core. Perhaps an add-on gives this proposal enough traction that folks can try to converge and standardize on it before committing more fully. That being said, I find this proposal better than https://github.com/facebook/react/issues/3858 and https://github.com/facebook/react/pull/3920 for its minimalism.

This is something I've been using on side projects (so grains of salt) - it's similar to @elierotenberg's awesome work but doesn't takeover the lifecycle, since this app isn't 100% in React and has to interop.

Brace yourself for CoffeeScript and mixins, or keep squinting until this looks like ES6 if you prefer. :)

_ = require 'lodash'

module.exports = DeclareNeedsMixin = 
  componentDidMount: ->
    @needsConsumerId = _.uniqueId @constructor.displayName
    @sinkNeeds @props, @state

  componentWillUpdate: (nextProps, nextState) ->
    @sinkNeeds nextProps, nextState

  componentWillUnmount: ->
    @props.flux.declareNeeds @needsConsumerId, []

  sinkNeeds: (props, state) ->
    if not @declareNeeds?
      return console.warn 'Missing method required for DeclareNeedsMixin: `declareNeeds`', @

    needs = @declareNeeds props, state
    props.flux.declareNeeds @needsConsumerId, needs

  # Intended to be overridden by the host class.
  # Returns a set of facts, stored as an array.
  # Yes, immutable data is awesome, that's not the point here though. :)
  # Facts are serializable data, just values.
  # declareNeeds: (props, state) ->
  #   []

And used like this:

module.exports = EmailThreads = React.createClass
  displayName: 'EmailThreads'
  mixins: [DeclareNeedsMixin]

  propTypes:
    flux: PropTypes.flux.isRequired

  declareNeeds: (props, state) ->
    [Needs.GmailData.myThreads({ messages: 20 })]

  ...

So declareNeeds is a one-way function of props and state to a description of what this component needs. In the actual implementation here, the receiving end of @props.flux.declareNeeds, which is set up at a top-level component, sinks these needs into a ProcessSink object. It batches as it likes, de-dupes needs across components that share the same flux, and then performs side-effects to meet those needs (like connecting to a socket or making HTTP requests). It uses reference counting to clean up stateful things like socket connections when there are no components who need them anymore.

Data flows through from stateful bits like socket events and requests into the dispatcher (and then to Stores or wherever) and back to components to meet their needs. This isn't synchronous and so all components handle rendering when data is not yet available.

I'm sharing this here only as another voice that exploring these kinds of solutions are possible in user-space, and that the current API is serving that kind of experimentation really well. In terms of a minimal step core could take to support interop between different approaches, I think @elierotenberg nailed it:

If anything, exposing and supporting maintaining React components instances lifecycle outside of a mounted React hierarchy would help.

kevinrobinson commented 9 years ago

As for the stateless approach, it seems to me that async data fetching is stateful, and thus storing the pending/completed/failed status in some components' state is relevant.

@elierotenberg and @andrewimm make an excellent point about first-class error handling. @sebmarkbage I think your instinct towards the minimal interop point is right on, but I'm not sure how adding semantics for errors to bubble up the component tree fits that requirement. There needs to be an equally simple story here for how to access values from onError and onCompleted, even if it's just that slots in this.observed holds the last value of either next/error/completed like { next: "foo" }. Without supporting the observable contract as a first-class feature, I'm a bit skeptical about this proposal making the cut.

And since this is the internet, and one of my first times chiming in here: the React issues feed is some of the best reading and an all-around source for great work and ideas. :+1:

StoneCypher commented 9 years ago

smells like bindings to me.

elierotenberg commented 9 years ago

I'm not sure how this relates to the current proposal/implementation, but I've found that enabling composition and higher-order manipulation is actually a key feature of data dependency tracking, especially if you use a reactive data source (flux, flux over the wire, or anything else than provides you with updates).

Take the following example with react-nexus@^3.4.0:

// the result from this query...
@component({
  users: ['remote://users', {}]
})
// is injected here...
@component(({ users }) =>
  users.mapEntries(([userId, user]) =>
    [`user:${userId}`, [`remote://users/${userId}/profile`, {}]]
  ).toObject()
))
class Users extends React.Component {
  // ... this component will receive all the users,
  // and their updates.
}

All in all, I wonder if there should be a data binding in the component API at all. It seems to me that higher-order-components-returning decorators provide a very nice way of expressing data binding without polluting the component methods namespace.

However, as @sebmarkbage noted, there is a risk of pollution the props namespace instead. For now, I'm using a props transformer decorator (react-transform-props) to clean up/rename props before passing them to the inner component, but I acknowledge it might become problematic in the future if higher-order-components become more commonplace and the risks of name clashing increases. Could this be solved by using symbols are property keys? Does propTypes checking support Symbol-keyed props? Will JSX support computed inline props keys (although one can always use computed properties + object spread)?

Sorry if this gets a little offtopic but it seems to me that we still have to find the right sweet abstraction/API for expressing data deps at the component level.

threepointone commented 9 years ago

my 2¢

I've been having much fun with the 'children as a function' pattern for values that change over time. I believe @elierotenberg first came up with it? My current usage is to model springs (via rebound) on react-springs. Example -

<Springs to={{x: 20, y: 30}} tension={30}>
  {val => <div style={{left: val.x, top: val.y}}>moving pictures</div>}
</Springs>

No props collision, and no usage of the owner's state. I can also nest multiple springs, and react manages all the hard bits. These 'observables' (ha!) could also accept onError, onComplete and other props (graphql queries?).

A rough attempt to sketch this out for 'flux'

<Store 
  initial={0}
  reduce={(state, action) => action.type === 'click'? state+1 : state} 
  action={{/* assume this comes as a prop from a 'Dispatcher' up somewhere */}}> 
    {state => <div onClick={() => dispatch({type: 'click'})}> clicked {state} times</div>}
</Store>
pspeter3 commented 9 years ago

We used this pattern, what we called render callbacks, at Asana but are ultimately moving away from it.

Pros

Cons

We're now moving to a model where the StoreComponent takes a ReactElement and when the store receives new data, the store clones the ReactElement overriding a specific prop. There are many ways to do this pattern such as taking a Component constructor and props. We went with this approach because it was the easiest to model in TypeScript.

threepointone commented 9 years ago

Terrific points, can't think of a way around either without breaking the 'sideways' requirement.

jimfb commented 9 years ago

@threepointone Sounds exactly like one of the implementation proposals to power https://github.com/reactjs/react-future/pull/28

threepointone commented 9 years ago

@pspeter3 in your example, is there a serious drawback to making Store always update / shouldComponentUpdate: ()=> true? I'm thinking its 'children' would have been rendered without Store in the chain anyway. Thanks for your time!

pspeter3 commented 9 years ago

@threepointone That is exactly what we did for our boundaries. It was unclear what the impact was exactly but there were perf worries from members of the team. The worries combined with the difficulty testing compelled the switch to using React.cloneElement(this.props.child, {data: this.state.data})

threepointone commented 9 years ago

@pspeter3 the testing angle is definitely a problem. What if shallowRenderer recognized 'render callbacks'? Would that help?

Ps- 'render callback' :thumbs_up:

tgriesser commented 9 years ago

@sebmarkbage the current discussion over on es-observable is that subscribe would be guarenteed async, with a [Symbol.observer] method provided to shortcut and subscribe synchronously, though the validity of having both sync/async apis is currently being debated.

I had chimed in on this ticket with the use case mentioned above in favor of synchronous subscription, didn't know if you might have anything to add there.

I think the ideas here are a very clean pattern for handling external state, though after playing around with it for a bit, I'm leaning in favor of the HOC approach.

Also @gaearon I simplified your HOC bit above, using a static - ComposedComponent.observe and using this.state rather than this.state.data - https://gist.github.com/tgriesser/d5d80ade6f895c28e659

It looks really nice with the proposed es7 decorators:

@observing
class Foo extends Component {
  static observe(props, context) {
    return {
      myContent: xhr(props.url)
    };
  }
  render() {
    var myContent = this.props.data.myContent;
    return <div>{myContent}</div>;
  }
}

The class decorator could even go so far as adding a getter for data to make it come very close to the original proposed API (minus local state affecting observable subscriptions, which I agree is a good thing - much less noise around subscribe / unsubscribe).

nmn commented 9 years ago

lack of synchronous subscription could be a huge problem.

lexfrl commented 9 years ago

I think I've got it how to solve "the state problem" and "Sideways Data Loading" (derivative data) in a consistent way. It makes it in a stateless "React-way". I've found a way how to maintain state consistency at any point of time and it fits the pattern UI = React(state). Unlikely I'm out of hands to make it completely bullet-proof, add more examples and make good presentation. https://github.com/AlexeyFrolov/slt . On other hand it's well tested and I'm using it in my production projects iteratively. Clever minds are welcome to contribute.

mweststrate commented 9 years ago

Hi all,

Its funny that I didn't stumble in this thread before, as at our company we were running into the same problems addressed by this proposal half a year ago. We started using React for a large scale project (think of an editor with the complexity of Microsoft Visio, with very cyclic data). Vanilla react countn't keep up with our performance demands, and flux was a bit of a no-go for us due its large amount of boilerplate and error proneness of all the subscriptions. So we figured out that we needed observable data structures as well.

As we couldn't find anything available that was ready to use, we built our own observables lib, based on the principles of the knockout observables (especially: automatic subscriptions). This fits actually very neatly in the current lifecycle of React components, and we didn't need weird hacks or even child rendering callbacks (the ObserverMixin used below is about 10 loc). This improved our DX a lot and worked so tremendously well for our team, that we decided to publish it as open source. In the mean time it is quite battle proven (it provides an ES7 observable array polyfill for example) and highly optimized. Here is a short timer example, (also available as JSFiddle), IMHO the DX couldn't be much better... :relieved:

var store = {};
// add observable properties to the store
mobservable.props(store, {
    timer: 0
});

// of course, this could be put flux-style in dispatchable actions, but this is just to demo Model -> View
function resetTimer() {
    store.timer = 0;
}

setInterval(function() {
    store.timer += 1;
}, 1000);

var TimerView = React.createClass({
    // This component is actually an observer of all store properties that are accessed during the last rendering
    // so there is no need to declare any data use, nor is there (seemingly) any state in this component
    // the combination of mobservable.props and ObserverMixin does all the magic for us.
    // UI updates are nowhere forced, but all views (un)subscribe to their data automatically
    mixins: [mobservable.ObserverMixin],

    render: function() {
        return (<span>Seconds passed: {this.props.store.timer}</span>);
    }
});

var TimerApp = React.createClass({
    render: function() {
        var now = new Date(); // just to demonstrate that TimerView updates independently of TimerApp
        return (<div>
            <div>Started rendering at: {now.toString()}</div>
            <TimerView {...this.props} />
            <br/><button onClick={resetTimer}>Reset timer</button>
        </div>);
    }
});

// pass in the store to the component tree (you could also access it directly through global vars, whatever suits your style)
React.render(<TimerApp store={store} />, document.body);

For more details about this approach, see this blog. BTW, I'll make sure a decorator and/or container will be added to the lib, for those using ES6 classes.

Saddly, I didn't see this thread before react-europe, otherwise we would have had a nice opportunity to discuss React & observables. But big thanks for the inspiring talks! :+1: I especially loved the abstractions of GraphQL and the thought work behind Redux :)

lexfrl commented 9 years ago

@mweststrate I feel community will end up with need to choose between the "Observables" and the "Immutable data" solutions ultimately. Maybe we need to mix in some way to have the strengths of both approach in the one solution (https://github.com/AlexeyFrolov/slt/issues/4). In my solution I've implemented the "Immutable data" approach with focus on the consistency of the state at any point of time. I'm planning to support Observables and Generators as well. This is an example of rules with is using to fetch "derivative" or "supplement" data (such a page body, assets, recommendations, comments, likes) and maintain the consistency of the app state.

https://github.com/AlexeyFrolov/slt#rules-example

It's a complex real-world (my production code) example that shows how to handle API redirects with a Location header.

import r from "superagent-bluebird-promise";
import router from "./router";

export default {
    "request": function (req)  {
        let route = router.match(req.url);
        let session = req.session;
        route.url = req.url;
        return this
            .set("route", route)
            .set("session", req.session);
    },
    "route": {
        deps: ["request"],
        set: function (route, request) {
            let {name, params: { id }} = route;
            if (name === "login") {
                return this;
            }
            let url = router.url({name, params: {id}});
            let method = request.method ? request.method.toLowerCase() : "get";
            let req = r[method]("http://example.com/api/" + url);
            if (~["post", "put"].indexOf(method)) {
                req.send(request.body);
            }
            return req.then((resp) => {
                let ctx = this.ctx;
                let path = url.substr(1).replace("/", ".");
                if (!resp.body) {
                    let location = resp.headers.location;
                    if (location) {
                        ctx.set("request", {
                            method: "GET",
                            url: location.replace('/api', '')
                        });
                    }
                } else {
                    ctx.set(path, resp.body);
                }
                return ctx.commit();
            });
        }
    }
}

In the rest it the same approach as yours, except the is no direct binding to the React (it's not required in my case). I believe we need to join forces somehow to archive the common goal.

YChebotaev commented 9 years ago

I think it's a bad idea because there will be many edge-cases that hard to consider.

From comments above i can list possible scopes that needs to be thinked of by end user each time he wants to create "dataful" component:

I think this proposal will lead to error prone, unstable, and hard to debug components.

I'm agree with proposed by @glenjamin connect and disconnect lifecycle hooks.