Netflix / falcor-router

A Falcor JavaScript DataSource which creates a Virtual JSON Graph document on your app server.
http://netflix.github.io/falcor
Apache License 2.0
104 stars 46 forks source link

Update RxJS to v5.0.0 #193

Closed benlesh closed 7 years ago

benlesh commented 7 years ago

EDIT: updated 12/02/17

This just updates this library to use RxJS 5. Primary motivation for this being debugging and performance.

NOTE: The use of Rx's expand had to have the Rx.Scheduler.queue scheduler passed to it explicitly, to insure the trampolining behavior that falcor-router was relying on. This was the default with Rx 4 and under. Overall, this is probably a good thing, as falcor router could theoretically be asked to crawl sufficiently large enough graphs to cause a stack overflow in some cases.

This just updates this library to use RxJS 5. Primary motivation for this being debugging and performance.

(edit, edit): no longer need Symbol.observable patching.

jhamlet commented 7 years ago

@blesh what's the work-a-round for getting the ModelResponse to work with RxJs@5+?

benlesh commented 7 years ago

@jhamlet you might be able to patch ModelResponse to have a proper implementation of Symbol.observable. (you can find one at https://github.com/blesh/symbol-observable) Otherwise, I'd wait for that PR to land.

trxcllnt commented 7 years ago

@blesh tbh expand scheduling on the queue scheduler only matters for routes which return data synchronously (values, or sync Observables). We're running the router in production on a massive domain graph, most of which is backed by synchronous caches, and haven't had any issues. The moment an async route is encountered (which usually happens at least once), the stack unwinds for any sync expands. In our experience, the queue scheduling is prohibitively expensive for running falcor router on the client, and produces nasty flame graphs for our servers. Perhaps the asap scheduler is more appropriate (for the client), though still produces difficult flame graphs.

benlesh commented 7 years ago

the queue scheduling is prohibitively expensive

@trxcllnt are you saying the queue itself gets expensive?

benlesh commented 7 years ago

FYI: @jhusain @trxcllnt I've updated this PR to reflect your requested changes

jhusain commented 7 years ago

LGTM