Closed
Bug 1242905
Opened 8 years ago
Closed 6 years ago
Move job rendering to React JS
Categories
(Tree Management :: Treeherder: Frontend, defect, P1)
Tree Management
Treeherder: Frontend
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: martianwars, Assigned: camd)
References
(Blocks 1 open bug)
Details
(Keywords: outreachy)
Attachments
(7 files)
47 bytes,
text/x-github-pull-request
|
wlach
:
feedback+
|
Details | Review |
47 bytes,
text/x-github-pull-request
|
camd
:
review+
|
Details | Review |
486.16 KB,
application/binary
|
Details | |
47 bytes,
text/x-github-pull-request
|
camd
:
review+
|
Details | Review |
47 bytes,
text/x-github-pull-request
|
camd
:
review+
|
Details | Review |
47 bytes,
text/x-github-pull-request
|
camd
:
feedback+
|
Details | Review |
47 bytes,
text/x-github-pull-request
|
emorley
:
feedback+
|
Details | Review |
This is an attempt to speed up the job rendering process by assigning the task to ReactJS classes.
Reporter | ||
Comment 1•8 years ago
|
||
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)
Reporter | ||
Updated•8 years ago
|
Assignee: nobody → kalpeshk2011
Comment 2•8 years ago
|
||
(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 3•8 years ago
|
||
Reporter | ||
Comment 4•8 years ago
|
||
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 5•8 years ago
|
||
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+
Updated•8 years ago
|
Priority: -- → P3
Assignee | ||
Comment 6•8 years ago
|
||
Moving this to our Outreachy intern.
Assignee: kalpeshk2011 → cwillia5
Mentor: cdawson
Reporter | ||
Comment 7•8 years ago
|
||
Good luck Casey! This might be helpful, https://github.com/martiansideofthemoon/treeherder/pull/1
Assignee | ||
Comment 8•8 years ago
|
||
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
Assignee | ||
Comment 9•8 years ago
|
||
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.
Assignee | ||
Comment 10•8 years ago
|
||
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
Comment 11•7 years ago
|
||
Updated•7 years ago
|
Attachment #8819424 -
Flags: review?(cdawson)
Assignee | ||
Comment 12•7 years ago
|
||
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)
Updated•7 years ago
|
Attachment #8819424 -
Flags: review?(cdawson)
Assignee | ||
Comment 13•7 years ago
|
||
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+
Comment 14•7 years ago
|
||
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
Comment 15•7 years ago
|
||
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)
Comment 16•7 years ago
|
||
(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)
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.
Comment 19•7 years ago
|
||
Comment 20•7 years ago
|
||
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)
Assignee | ||
Comment 21•7 years ago
|
||
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)
Assignee | ||
Comment 22•7 years ago
|
||
Comment on attachment 8821367 [details] [review] [treeherder] caseywilliams:react > mozilla:master Works well. Nice work. :)
Attachment #8821367 -
Flags: review?(cdawson) → review+
Comment 23•7 years ago
|
||
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.
Comment 24•7 years ago
|
||
Updated•7 years ago
|
Attachment #8823330 -
Flags: review?(cdawson)
Assignee | ||
Comment 25•7 years ago
|
||
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-
Updated•7 years ago
|
Attachment #8823330 -
Flags: review- → review?(cdawson)
Assignee | ||
Comment 26•7 years ago
|
||
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+
Comment 27•7 years ago
|
||
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.
Comment 28•7 years ago
|
||
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.
Comment 29•7 years ago
|
||
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.
Comment 30•7 years ago
|
||
Comment 31•7 years ago
|
||
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.
Updated•7 years ago
|
Attachment #8831740 -
Flags: feedback?(eperelman)
Attachment #8831740 -
Flags: feedback?(cdawson)
Assignee | ||
Comment 32•7 years ago
|
||
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 33•7 years ago
|
||
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)
Updated•7 years ago
|
Blocks: treeherder-react
Updated•7 years ago
|
Component: Treeherder → Treeherder: Frontend
Comment 34•6 years ago
|
||
Assignee | ||
Comment 35•6 years ago
|
||
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 36•6 years ago
|
||
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+
Updated•6 years ago
|
Assignee: cwillia5 → cdawson
Mentor: cdawson, wlachance
Status: NEW → ASSIGNED
Updated•6 years ago
|
Priority: P3 → P1
Comment 37•6 years ago
|
||
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
Updated•6 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Comment 38•6 years ago
|
||
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•