Closed Bug 1242905 Opened 8 years ago Closed 6 years ago

Move job rendering to React JS

Categories

(Tree Management :: Treeherder: Frontend, defect, P1)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: martianwars, Assigned: camd)

References

(Blocks 1 open bug)

Details

(Keywords: outreachy)

Attachments

(7 files)

This is an attempt to speed up the job rendering process by assigning the task to ReactJS classes.
William,
I investigated a little and managed to replace the entire UI with a "Hello World" rendered by ReactJS (without JSX). I think using JSX is the way to go, but not too sure about the correct technique / pattern to be followed. JSX needs to be added under a <script type="text/babel"> and hence cannot be directly added to the clonejobs.js directive. I guess the correct place is next to the "text/ng-template" script. What is the right way of importing it into the directive? What do you think about [2] and [3]? [3] seems to me the right way to go.
[1] seems to be what we are aiming at, but uses vanilla JS through and through. 


[1] - http://www.williambrownstreet.net/blog/2014/04/faster-angularjs-rendering-angularjs-and-reactjs/ 
[2] - https://github.com/ngReact/ngReact
[3] - https://github.com/wesleycho/angular-react
Flags: needinfo?(wlachance)
Assignee: nobody → kalpeshk2011
Blocks: 1067846
(In reply to Kalpesh Krishna [:martianwars] from comment #1)
> William,
> I investigated a little and managed to replace the entire UI with a "Hello
> World" rendered by ReactJS (without JSX). I think using JSX is the way to
> go, but not too sure about the correct technique / pattern to be followed.
> JSX needs to be added under a <script type="text/babel"> and hence cannot be
> directly added to the clonejobs.js directive. I guess the correct place is
> next to the "text/ng-template" script. What is the right way of importing it
> into the directive? What do you think about [2] and [3]? [3] seems to me the
> right way to go.
> [1] seems to be what we are aiming at, but uses vanilla JS through and
> through. 
> 
> 
> [1] -
> http://www.williambrownstreet.net/blog/2014/04/faster-angularjs-rendering-
> angularjs-and-reactjs/ 
> [2] - https://github.com/ngReact/ngReact
> [3] - https://github.com/wesleycho/angular-react

Yeah in principle it looks like [2] or [3] makes sense. I think [1] is more just showing that such a thing is possible, rather than the best practice for how to do it. :)
Flags: needinfo?(wlachance)
Comment on attachment 8712634 [details] [review]
[treeherder] martiansideofthemoon:lugia > mozilla:master

William,
Just a start. Wanted your feedback before I try to expand on this. I'm having a hard time integrating [2] and [3]. What do you think?
Attachment #8712634 - Flags: feedback?(wlachance)
Comment on attachment 8712634 [details] [review]
[treeherder] martiansideofthemoon:lugia > mozilla:master

This looks like a good start to me. The trick will be figuring out how to integrate this in clonejobs. Hopefully it won't be too difficult. :)
Attachment #8712634 - Flags: feedback?(wlachance) → feedback+
Priority: -- → P3
Keywords: outreachy
Moving this to our Outreachy intern.
Assignee: kalpeshk2011 → cwillia5
Mentor: cdawson
Good luck Casey! This might be helpful, https://github.com/martiansideofthemoon/treeherder/pull/1
I think a good approach to this will be to go "from the inside out".  So begin by replacing the smaller details of rendering with react components, then move outward.  In general, I'd like to entirely replace the directive of ``clonejobs.js`` with react.

We should create new ReactJS components replacing clonejobs functions in this order:

1. ``addRevisions`` - render the revisions list for a resultset.
    a. ``toggleRevisions`` - toggle visibility of revisions

2. jobs and counts: - render each platform for a resultset
    a. replace functions of ``renderGroupJobsAndCounts`` and ``getJobBtnEls``
    c. Re-render when new jobs come in / filters applied / jobs clicked, etc.

3. ``generateJobElements`` / ``getJobTableRowHTML`` - render all platforms for a resultset
Unfortunately, the organization of clonejobs.js has gotten kind of bad.  But as we transition to ReactJS components, we should be able to peel away parts of it, making it easier to follow in later steps.

Starting with ``addRevisions`` will be a fairly contained and simpler version of the other steps.  I'm hoping this will be a good way to get your feet wet with ReactJS and help us establish good patterns and organization going forward.
Also, let's see how far we can get NOT using JSX.  Let's use ReactDOM directly for now.  Please see this part of the code as an example of a react component in use:

https://github.com/mozilla/treeherder/blob/a4c0b7a3b492c25dc8357a7d62afded4faaba14b/ui/js/reactselect.js#L3-L3
Attachment #8819424 - Flags: review?(cdawson)
Comment on attachment 8819424 [details] [review]
[treeherder] caseywilliams:react > mozilla:master

I'll just clear the review for now since I asked for such minor changes.  :)
Attachment #8819424 - Flags: review?(cdawson)
Attachment #8819424 - Flags: review?(cdawson)
Comment on attachment 8819424 [details] [review]
[treeherder] caseywilliams:react > mozilla:master

Looks great!  Will test on stage before merge.
Attachment #8819424 - Flags: review?(cdawson) → review+
Commit pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/676e1f4738dd2ddc8fcf6aeb525d7fb13409ae73
Bug 1242905 - Revision list react component (#2029)

Begins migration of job rendering to react by replacing cloned revision
lists with a new react component. Related changes:

- Prepends the ignore-job-on-click attribute with data- so that react
  will render it
- Makes the linkifyBugs filter wrap its html attributes in quotes
  consistently
- Adds explict whitespace via CSS in a few places that previously
  depended on whitespace in markup
I have been noticing a weird lag between clicking on a job and the information coming up on stage since this landed.

Method to reproduce:

1. Load treeherder
2. Click on a job symbol

On production this happens almost instantaneously. On stage (where this patch is applied), it takes between 0.5 to several seconds for the panel to open. Testing locally, reverting this push fixes things.

Casey, could you investigate, maybe by using the Firefox or Chrome performance measurement tools? I think we should probably back this out if we can't figure it out, as this has a significant usability impact.
Flags: needinfo?(cwillia5)
(In reply to William Lachance (:wlach) from comment #15)

> Casey, could you investigate, maybe by using the Firefox or Chrome
> performance measurement tools? I think we should probably back this out if
> we can't figure it out, as this has a significant usability impact.

s/figure it out/figure it out quickly/ I have no doubt there is a solution here, just that we should back out for now if it isn't immediately apparent what it is (since we can always just reland this later)
Attached file profile.json
Captured a performance profile, looks like a click event is taking 1128ms, with most of that being a bunch of little "Minor GC"s. Then a bit after that, there's a setTimeout that takes 1762ms, most of which being even more little "Minor GC"s.

No clue if that helps out.
But yeah, the performance of the panel is not acceptable as-is, so this either needs fixed or backed out before the next prod push.
It turns out that ngReact uses deep watches on component props by default, so I suspect that that's what was causing the lag. The responsiveness seems much improved (on par with prod) after modifying the watch type.
Flags: needinfo?(cwillia5)
Comment on attachment 8821367 [details] [review]
[treeherder] caseywilliams:react > mozilla:master

Marking myself to review this.  I'll get to it shortly.  :)
Attachment #8821367 - Flags: review?(cdawson)
Comment on attachment 8821367 [details] [review]
[treeherder] caseywilliams:react > mozilla:master

Works well.  Nice work.  :)
Attachment #8821367 - Flags: review?(cdawson) → review+
Commit pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/72f3605f82e1cb986c97d4f76673588d11c8e1d1
Bug 1242905 - Fix watch depth in react revisions (#2041)

By default, ngReact watches component props using an expensive deep
$watch. This made the revision list react components noticeably slow
down the UI when clicking a job button. This commit changes the watch
depth for the revision list components to lighten the load.
Attachment #8823330 - Flags: review?(cdawson)
Comment on attachment 8823330 [details] [review]
[treeherder] caseywilliams:react > mozilla:master

Left some review comments.  This is super close!  :)
Attachment #8823330 - Flags: review?(cdawson) → review-
Attachment #8823330 - Flags: review- → review?(cdawson)
Comment on attachment 8823330 [details] [review]
[treeherder] caseywilliams:react > mozilla:master

These look good!  Tested quite a bit locally and everything seems on parity with production.  No speed increase at this point, but that's not unexpected.  The real increase will come with doing the actual jobs.

Great work!!
Attachment #8823330 - Flags: review?(cdawson) → review+
Commits pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/68b475335336cf12076b18d074f8f4cb43211153
Bug 1242905 - Use ngreact-test-utils for ngreact tests

Introduces the ngreact-test-utils npm package to supply reusable
compile() and simulate() functions for testing ngreact components. This
requires browserify, which is now applied to tests in
tests/ui/unit/react only.

https://github.com/mozilla/treeherder/commit/9d54ed77c7d8eaeb27ee3bb03175be223b28c129
Bug 1242905 - React component for plaform TDs

Replaces TD elements for platforms in job lists with a new react
component.
Depends on: 1331056
Commit pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/8c2b39c76ef4e487bd5b75d69e2920d4797d89ee
Revert "Bug 1242905 - React component for plaform TDs"

This reverts commit 9d54ed77c7d8eaeb27ee3bb03175be223b28c129.
I had to revert some of the more recent work on this because of bug 1331056, which indicates a leak somewhere in this code. 

I don't know if this is the full culprit (I suspect it isn't), but one thing that stood out to me taking a quick look was this line:

https://github.com/mozilla/treeherder/blob/9d54ed77c7d8eaeb27ee3bb03175be223b28c129/ui/js/directives/treeherder/clonejobs.js#L778

In an earlier optimization pass, we had found that creating temporary DOM nodes using jQuery was pretty inefficient and not necessary for what we're trying to do here. It would be better to just concatenate the HTML string, and set the innerhtml of whatever element you want to hang it off of.
Some comments on my last commit:

This commit replaces the rendering and interactions previously handled by clonejobs.js with React components. It seems to me to have the functionality present on the master branch, barring a few exceptions, listed below. It is not ready to merge, but I’m hoping for some feedback that may help with a few things that have been concerning me over the past week.

Problems:

1 - Previously, I had attempted to replace the clonejobs rendering from the bottom up, replacing more deeply nested elements with ngReact components incrementally. This actually hurt load times, since ngReact had to render many (hundreds, even) react roots. 

This commit takes the approach of replacing all of clonejobs at once, leaving the page with only as many react roots as there are resultsets. Load time is improved over my initial replacement attempts, but is no better than the current master branch. I am not sure how to improve on this, but there are probably many ways this code could be optimized that I don’t know about.

Memory usage around 25% higher for this React version vs the clonejobs version, which may be unavoidable because of the virtual dom, but again, I’m hoping there are some places in my code that I can reduce this. 

2 - Keyboard shortcuts for navigating between selected jobs presented a significant problem. Previously, when using the right/left arrow keys or j/k to move between jobs, jQuery selectors were used to find the next visible job button for selection. But in react, job selection status is controlled by the component’s state — changing this from the outside requires access to the component object, which (as far as I can tell) can’t be accessed by selector from outside the tree of react components. 

The workaround I used for this feature in this commit is awkward, convoluted, and verbose - it communicates between parent and child elements using refs and context, traversing the edges of the tree of components until it finds an adjacent visible job button. This seemed like my only option because jobs are grouped into counts and individual buttons based on global filters at the point when they are rendered (i.e. not on the server side or on the root resultset, for example). 

I can’t think of an obviously better way to do this, but I think I’d try to introduce some sort of state container that could be available to all components at the same time, and use that to hold all displayed data in one place at the react root, including the UI state for all the job and group objects (which right now is computed inside child components). This state container could perhaps be a simple context object, or an actual flux implementation like redux (which uses context anyway). I’m very interested to hear what others think.

3 - Finally, using context makes testing hard when using the built-in React.addons.TestUtils, since React.createElement() will not take a context argument the way it takes props. I couldn’t see a way to *not* use context and still manage to implement all the features, though. Testing with context would be made easy if we could use the enzyme test framework, but as far as I can tell, enzyme requires jsx and node module support. It would be nice to be able to use e.g. webpack to bundle assets for treeherder so that we could do this, but I don’t know how much of a rabbit hole that would be or even if it would be possible without significant overhaul.

Missing features:

- The fade effect on job group expand/collapse is much trickier when using React than when using jQuery. I was able to get a (mostly) working fade effect using CSSTransitionGroup from React.addons, but that required using the expanded react-with-addons.js library, as opposed to plain react.js. If that’s okay, I’ll go ahead and finish it up, otherwise, I’m not sure how to successfully animate the expand/collapse action.

- The viewport does not automatically scroll to center the currently selected job if the job is not visible — I haven’t run into any specific problems on this front, but I was anxious to get some feedback on the larger problems described above before continuing.
Attachment #8831740 - Flags: feedback?(eperelman)
Attachment #8831740 - Flags: feedback?(cdawson)
Comment on attachment 8831740 [details] [review]
[treeherder] caseywilliams:react > mozilla:master

Casey walked me through the code in Vidyo.  It looks really great.  Had a few comments I gave verbally:

1. More comments for functions and components
2. Use { and } for simple if-statements even though they CAN go on one line
3. Try using the events ``jobClick`` and ``clearSelectedJob`` for detecting which job is selected instead of an watch
4. Try out using ``let`` instead of ``var`` where possible.
Attachment #8831740 - Flags: feedback?(cdawson) → feedback+
Comment on attachment 8831740 [details] [review]
[treeherder] caseywilliams:react > mozilla:master

I walked through the code, but I am not very familiar with the code style nor the intricacies of bridging between Angular and React, so I'm afraid I'm not much help here.
Attachment #8831740 - Flags: feedback?(eperelman)
Component: Treeherder → Treeherder: Frontend
Comment on attachment 8946739 [details] [review]
Link to GitHub pull-request: https://github.com/mozilla/treeherder/pull/3177

We already went over this on Vidyo, but in case you'd like to give it your blessing.  :)
Attachment #8946739 - Flags: review?(emorley)
Comment on attachment 8946739 [details] [review]
Link to GitHub pull-request: https://github.com/mozilla/treeherder/pull/3177

This looks great! I've left some comments on the PR - next we should test it out some more on prototype (or else stage), after the Auth0 testing there is wrapped up.
Attachment #8946739 - Flags: review?(emorley) → feedback+
Assignee: cwillia5 → cdawson
Mentor: cdawson, wlachance
Status: NEW → ASSIGNED
Priority: P3 → P1
Commit pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/f40f3faae74c02ba88a1ceefc1675dc1a7f969c2
Bug 1242905 - Replace rendering of pushes and jobs with ReactJS

This replaces the logic that was in clonejobs.js that formerly
handled rendering the pushes, platforms and jobs and converts it to
ReactJS.  The ReactJS code is hosted as a directive in AngularJS
using ngReact reactDirective.

This also removes the feature where you can hide revisions because
it was believed to not be used and added unnecessary complications
to the code.

Co-authored-by: Casey Williams cwillia5@gmail.com
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Depends on: 1438315
Commit pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/236612a34be70d809e9a29e255235a833347fd8a
Bug 1242905 - Fix honoring group_state url param on load

Fixes a bug found after deploy.
Depends on: 1438410
Depends on: 1438439
Depends on: 1438555
Depends on: 1438718
Depends on: 1444931
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: