Closed Bug 1349288 Opened 7 years ago Closed 7 years ago

Add vendor dependencies React and Redux to Activity-Stream system add-on

Categories

(Firefox Graveyard :: Activity Streams: General, enhancement)

Product:
Firefox Graveyard
Component:
Activity Streams: General
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: k88hudson, Assigned: k88hudson)

References

Details

Attachments

(2 files)

Bug 1349288 - Update license to allow activity-stream to use React, Redux, Reselect, and ReactRedux
59 bytes, text/x-review-board-request
gerv
: review+
standard8
: review+
Details
Bug 1349288 - Add Redux and React files to activity-stream extension.
59 bytes, text/x-review-board-request
standard8
: review+
Details 
We need our own versions of React and Redux added to the Activity Stream add-on.
Assignee: nobody → khudson
Ok, after looking at what the devtools folks have done, I've added the necessary license changes for about:license, and added copies of each file to a vendor directory in the activity-stream extension.
Blocks: 1350409
Comment on attachment 8851020 [details]
Bug 1349288 - Update license to allow activity-stream to use React, Redux, Reselect, and ReactRedux

https://reviewboard.mozilla.org/r/123482/#review126778

::: toolkit/content/license.html
(Diff revision 1)
>      <hr>
>  
>      <h1><a id="react-redux"></a>React-Redux License</h1>
>  
> -    <p>This license applies to the file
> +    <p>This license applies to various files in the Mozilla codebase.</p>
> -    <span class="path">devtools/client/shared/vendor/react-redux.js</span>.</p>

This seems a bit like a cop-out; is it really not possible to enumerate the files, or list the directories covered? I know we occasionally do this, but only when the list would be prohibitively long.
Attachment #8851020 - Flags: review?(gerv) → review-
Comment on attachment 8851020 [details]
Bug 1349288 - Update license to allow activity-stream to use React, Redux, Reselect, and ReactRedux

https://reviewboard.mozilla.org/r/123482/#review127976

r=gerv.
Attachment #8851020 - Flags: review?(gerv) → review+
Comment on attachment 8851021 [details]
Bug 1349288 - Add Redux and React files to activity-stream extension.

https://reviewboard.mozilla.org/r/123484/#review128004

I think this is generally OK. At some stage it feels like we're going to need a conversation about harmonising react/other libraries across the tree, but since you're currently using different versions from devtools (and we had separate ones for Hello), I think we'll ignore that for now.

::: commit-message-94be3:1
(Diff revision 2)
> +Bug 1349288 - Add Redux and React files to activity-stream extension.

Will these files be generally updated on importing new versions of activity stream?

If they're going to be imported then I think you should have a README.md at the top level saying that all the files are imported from the activity stream repo (and point to it).

If they aren't, then you should have some sort of document on how to upgrade them.

::: browser/extensions/activity-stream/vendor/react-dom.js:2
(Diff revision 2)
> +/**
> + * ReactDOM v15.4.2

I think we should allow for a debug version of this file, similar to how devtools does it:

https://dxr.mozilla.org/mozilla-central/rev/8df9fabf2587b7020889755acb9e75b664fe13cf/devtools/client/shared/vendor/moz.build#14

I'm fine if you don't do the build bit at the moment, but it would be nice to get the intention of the debug file in here.
Attachment #8851021 - Flags: review?(standard8)
Comment on attachment 8851021 [details]
Bug 1349288 - Add Redux and React files to activity-stream extension.

https://reviewboard.mozilla.org/r/123484/#review128004

I definitely agree it would be good to have that conversation – like you said, we're currently on a different version of React, and we're thinking it's not a good idea to be tightly coupled to the version Devtools is using.
Comment on attachment 8851021 [details]
Bug 1349288 - Add Redux and React files to activity-stream extension.

https://reviewboard.mozilla.org/r/123484/#review128004

> I think we should allow for a debug version of this file, similar to how devtools does it:
> 
> https://dxr.mozilla.org/mozilla-central/rev/8df9fabf2587b7020889755acb9e75b664fe13cf/devtools/client/shared/vendor/moz.build#14
> 
> I'm fine if you don't do the build bit at the moment, but it would be nice to get the intention of the debug file in here.

Ok, I added the files -- I will make a note to add the build step in a subsequent ticket
Comment on attachment 8851021 [details]
Bug 1349288 - Add Redux and React files to activity-stream extension.

https://reviewboard.mozilla.org/r/123484/#review128112
Attachment #8851021 - Flags: review?(standard8) → review+
Keywords: checkin-needed
Autoland can't push this because it doesn't see gerv's r+ in MozReview as sufficient for landing.
Keywords: checkin-needed
Attachment #8851020 - Flags: review?(dtownsend)
Attachment #8851020 - Flags: review?(dtownsend) → review?(standard8)
Comment on attachment 8851020 [details]
Bug 1349288 - Update license to allow activity-stream to use React, Redux, Reselect, and ReactRedux

https://reviewboard.mozilla.org/r/123482/#review128408

Stealing review, as gerv's already reviewed this, I'll rubber-stamp it for landing purposes.
Attachment #8851020 - Flags: review?(standard8) → review+
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e22dff1f7912
Update license to allow activity-stream to use React, Redux, Reselect, and ReactRedux r=gerv,standard8
https://hg.mozilla.org/integration/autoland/rev/9bf5112d0dbf
Add Redux and React files to activity-stream extension. r=standard8
https://hg.mozilla.org/mozilla-central/rev/e22dff1f7912
https://hg.mozilla.org/mozilla-central/rev/9bf5112d0dbf
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Depends on: 1344319
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: