Closed Bug 1312481 Opened 3 years ago Closed 3 years ago

Add React Virtualized

Categories

(DevTools :: Shared Components, defect, P2)

defect

Tracking

(firefox52 fixed)

RESOLVED FIXED
Firefox 52
Tracking Status
firefox52 --- fixed

People

(Reporter: linclark, Assigned: linclark)

References

(Blocks 2 open bugs)

Details

Attachments

(3 files, 3 obsolete files)

This will be useful for multiple tools, including web console (Bug 1308216) and net monitor (Bug 1308441)
Blocks: 1308216, 1308441
Attached patch Bug1312481-part1.patch (obsolete) — Splinter Review
Gerv, could you please give a license review for this external library that we're adding? react-addons-shallow-compare is part of React, so we already have the license for that. React Virtualized is the one that we need to add a license for.
Attachment #8804093 - Flags: review?(odvarko)
Attachment #8804093 - Flags: review?(gerv)
Attached patch Bug1312481-part2.patch (obsolete) — Splinter Review
Attachment #8804094 - Flags: review?(odvarko)
Assignee: nobody → lclark
Priority: -- → P2
According to elvin, dcamp is currently thinking about React usage within Mozilla. Before I r= this I'd like him to comment.

Gerv
Hi Gerv,

My understanding - this is complex and has been under discussion for 6+ months already, and React Virtualized isn't material to the React discussion

I'm keen for us to not hold up important work on devtools because of a side issue to a long running debate. Even if the answer to react is 'no' then we won't regret the decision to go ahead with React Virtualized.
Joe: AIUI, React Virtualized is additional components for React. If you want them in the codebase, my assumption is that you want to use them, and that this would increase our use of React. I don't feel I can give an r= to that without dcamp's approval - which he is of course at liberty to give while discussions about React continue, if he feels it's appropriate.

Gerv
I don't think it would increase our usage of React, perhaps the opposite! The alternative to React Virtualized isn't "write that component without using React", it's "write our own version of React Virtualized".
Like I said, just get Dave to give it the nod, and all will be well :-) If he doesn't, you can ask him what to use instead :-)

Gerv
Flags: needinfo?(dcamp)
a+
Flags: needinfo?(dcamp)
Comment on attachment 8804093 [details] [diff] [review]
Bug1312481-part1.patch

Review of attachment 8804093 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Lin for doing this!

R+ assuming my comment is resolved.

Honza

::: devtools/client/shared/vendor/react-addons-shallow-compare.js
@@ +1,1 @@
> +module.exports = require("devtools/client/shared/vendor/react").addons.shallowCompare;
\ No newline at end of file

Please put Mozilla license headers at the top.
Attachment #8804093 - Flags: review?(odvarko) → review+
Comment on attachment 8804094 [details] [diff] [review]
Bug1312481-part2.patch

Review of attachment 8804094 [details] [diff] [review]:
-----------------------------------------------------------------

We have an extra license files for react libs in the directory (e.g. REDUX_LICENSE and REACT_REDUX_LICENSE).
I think we should have an extra file for react-virtualized too: REACT_VIRTUALIZED_LICENSE

Honza
Attachment #8804094 - Flags: review?(odvarko)
One more thing, I think that the REACT_VIRTUALIZED_UPGRADING file should also have Mozilla license header at the top (I know that the other *_UPGRADING files are missing that).

Honza
Comment on attachment 8804093 [details] [diff] [review]
Bug1312481-part1.patch

Review of attachment 8804093 [details] [diff] [review]:
-----------------------------------------------------------------

r=gerv.

Gerv
Attachment #8804093 - Flags: review?(gerv) → review+
Attached patch Bug1312481-part2.patch (obsolete) — Splinter Review
I added the Mozilla license to the shallow compare file. On the other comments, it seemed like more files in the vendor directory did not do it that way. If we want to change that, I think it would make sense to file an issue and do them all together to make them consistent.
Attachment #8804094 - Attachment is obsolete: true
Attachment #8806166 - Flags: review+
Keywords: checkin-needed
(In reply to Lin Clark [:linclark] from comment #13)
> Created attachment 8806166 [details] [diff] [review]
> Bug1312481-part2.patch
> 
> I added the Mozilla license to the shallow compare file. On the other
> comments, it seemed like more files in the vendor directory did not do it
> that way. If we want to change that, I think it would make sense to file an
> issue and do them all together to make them consistent.
Done, bug 1314539

Honza
needs rebasing 

applying Bug1312481-part1.patch
patching file devtools/client/shared/vendor/moz.build
Hunk #1 FAILED at 1
1 out of 1 hunks FAILED -- saving rejects to file devtools/client/shared/vendor/moz.build.rej
patching file toolkit/content/license.html
Hunk #1 FAILED at 129
1 out of 2 hunks FAILED -- saving rejects to file toolkit/content/license.html.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh Bug1312481-part1.patch
Flags: needinfo?(lclark)
Keywords: checkin-needed
Attachment #8804093 - Attachment is obsolete: true
Flags: needinfo?(lclark)
Attachment #8807342 - Flags: review+
Attachment #8806166 - Attachment is obsolete: true
Since this has been in review for a while, I'm going to update it to a more recent version
Attachment #8807343 - Flags: review+
Attachment #8807347 - Flags: review+
Keywords: checkin-needed
Status: NEW → ASSIGNED
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/da601059fdb2
Part 1: Add react-virtualized. r=Honza
https://hg.mozilla.org/integration/mozilla-inbound/rev/f12283aa3114
Part 2: Make it possible to load React Virtualized in devtools (including XUL-based tools). r=Honza
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f53c8537285
Update react-virtualized to 8.3.1. r=bgrins
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/da601059fdb2
https://hg.mozilla.org/mozilla-central/rev/f12283aa3114
https://hg.mozilla.org/mozilla-central/rev/2f53c8537285
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.