Page MenuHomePhabricator

Provide a global router for Ajax requests
ClosedPublic

Authored by epriestley on May 5 2014, 12:42 AM.
Tags
None
Referenced Files
F14352865: D8979.diff
Thu, Dec 19, 3:02 PM
Unknown Object (File)
Tue, Dec 17, 11:11 AM
Unknown Object (File)
Tue, Dec 10, 6:59 PM
Unknown Object (File)
Wed, Dec 4, 7:50 AM
Unknown Object (File)
Wed, Nov 27, 7:51 AM
Unknown Object (File)
Mon, Nov 25, 4:44 AM
Unknown Object (File)
Sat, Nov 23, 3:47 PM
Unknown Object (File)
Thu, Nov 21, 11:30 AM
Subscribers

Details

Summary

Fixes T430. Fixes T4834. Obsoletes D7641. Currently, we do some things less-well than we could:

  • We just let the browser queue and prioritize requests, so if you load a revision with 50 changes and then click "Award Token", the action blocks until the changes load in most/all browsers. It would be better to prioritize this action and queue it immediately.
  • Similarly, changes tend to load in order, even if the user has clicked to a specific file. When the user expresses a preference for a specific file, we should prioritize it.
  • We show a spinning GIF when waiting on requests. This is appropriate for some types of reuqests, but distracting for others.

To fix this:

  • Queue all (or, at least, most) requests into a new queue in JX.Router.
  • JX.Router handles prioritizing the requests. Principally:
    • You can submit a request with a specific priority (500 = general content loading, 1000 = default, 2000 = explicit user action) and JX.Router will get the higher stuff fired off sooner.
    • You can name requests and then adjust their prorities later, if the user expresses an interest in specific results.
  • Only use the spinner gif for "workflow" requests, which is bascially when the user clicked something and we're waiting on the server. I think it's useful and not-annoying in this case.
  • Don't show any status for draft requests.
  • For content requests, show a subtle hipster-style top loading bar.
Test Plan
  • Viewed a diff with 93 changes, and clicked award token.
    • Prior to this patch, the action took many many seconds to resolve.
    • After this patch, it resolves quickly.
  • Viewed a diff with 93 changes and saw a pleasant subtle hipster-style loading bar.
  • Viewed a diff with 93 changes and typed some draft text. Previews populated fairly quickly and there was no spinner.
  • Viewed a diff with 93 changes and clicked something with workflow, saw a spinner after a moment.
  • Viewed a diff with 93 changes and clicked a file in the table of contents near the end of the list.
    • Prior to this patch, it took a long time to show up.
    • After this patch, it loads directly.

Diff Detail

Repository
rP Phabricator
Branch
router
Lint
Lint Errors
SeverityLocationCodeMessage
Errorwebroot/rsrc/externals/javelin/lib/Request.js:314JSHintW054JSHintW054
Unit
No Test Coverage
Build Status
Buildable 245
Build 245: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

epriestley retitled this revision from to Provide a global router for Ajax requests.
epriestley updated this object.
epriestley edited the test plan for this revision. (Show Details)
epriestley added reviewers: btrahan, chad.
  • Specifically set timeouts for ShapedRequest when the request fires, not when it is queued. This diff solves T430 anyway, but we can do this correctly for cheap.
  • Slightly simpler timeout fix; on consideration, we get it for free.
  • Route Aplict requests at very low priority.
btrahan edited edge metadata.

I don't know about this "hipster-style" thing, but otherwise LGTM.

webroot/rsrc/js/core/behavior-workflow.js
55

maybe this should be built in to the Workflow constructor?

This revision is now accepted and ready to land.May 5 2014, 5:01 PM
epriestley updated this revision to Diff 21323.

Closed by commit rP2b4c551b0ef9 (authored by @epriestley).

Oops, typed half a comment and then lost my window. Here was my reply:


Yeah, I think in the medium term this will either move to Workflow or maaybe to some PhabricatorHelperStuff-sort of class.

A mild argument against putting it in Workflow is that there are some cases where we use Workflow (normally for the side effect of having nice error messages) but don't want to give the request "the user clicked this, do it right now!" priority. However, we could still just adjust priority afterward. I'm also not totally sure that Router should go in the core -- instead of getRoutable(), the API could look more like Routable.newFromWorkflow(). All the arguments for either pushing it more into the core or pulling it further out of the core feel pretty mushy to me right now.

I think/hope that as a few more things move over it will be more clear where to put it.

CC'd you on T4834 for hipster bar screenshot / discussion.