1312481 - Add React Virtualized

Status: RESOLVED FIXED

(DevTools :: Shared Components, defect, P2)

Description

[Lin Clark [:linclark]]

This will be useful for multiple tools, including web console (Bug 1308216) and net monitor (Bug 1308441)

Comment 1

[Lin Clark [:linclark]]

Attachment: Bug1312481-part1.patch

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.

Comment 2

[Lin Clark [:linclark]]

Attachment: Bug1312481-part2.patch

Comment 3

[Gervase Markham [:gerv]]

According to elvin, dcamp is currently thinking about React usage within Mozilla. Before I r= this I'd like him to comment.

Gerv

Comment 4

[Joe Walker [:jwalker]]

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.

Comment 5

[Gervase Markham [:gerv]]

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

Comment 6

[Joe Walker [:jwalker]]

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".

Comment 7

[Gervase Markham [:gerv]]

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

Comment 8

[Dave Camp (:dcamp)]

a+

Comment 9

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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.

Comment 10

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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

Comment 11

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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 12

[Gervase Markham [:gerv]]

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

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

r=gerv.

Gerv

Comment 13

[Lin Clark [:linclark]]

Attachment: 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.

Comment 14

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(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

Comment 15

[Carsten Book [:Tomcat]]

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

Comment 16

[Lin Clark [:linclark]]

Attachment: Bug1312481-part1.patch

Comment 17

[Lin Clark [:linclark]]

Attachment: Bug1312481-part2.patch

Comment 18

[Lin Clark [:linclark]]

Attachment: Bug1312481-part3.patch

Since this has been in review for a while, I'm going to update it to a more recent version

Comment 19

[Pulsebot]

Pushed by [e-mail]:
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

Comment 20

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/da601059fdb2
https://hg.mozilla.org/mozilla-central/rev/f12283aa3114
https://hg.mozilla.org/mozilla-central/rev/2f53c8537285