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

RESOLVED FIXED in Firefox 55

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: k88hudson, Assigned: k88hudson)

Tracking

unspecified
Firefox 55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

2 years ago
We need our own versions of React and Redux added to the Activity Stream add-on.
(Assignee)

Updated

2 years ago
Assignee: nobody → khudson
(Assignee)

Comment 1

2 years ago
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.
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
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 8

2 years ago
mozreview-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)
(Assignee)

Comment 9

2 years ago
mozreview-review-reply
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 hidden (mozreview-request)
(Assignee)

Comment 11

2 years ago
mozreview-review-reply
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 12

2 years ago
mozreview-review
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+
(Assignee)

Updated

2 years ago
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
(Assignee)

Updated

2 years ago
Attachment #8851020 - Flags: review?(dtownsend)
Attachment #8851020 - Flags: review?(dtownsend) → review?(standard8)

Comment 14

2 years ago
mozreview-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/#review128408

Stealing review, as gerv's already reviewed this, I'll rubber-stamp it for landing purposes.
Attachment #8851020 - Flags: review?(standard8) → review+

Comment 15

2 years ago
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

Comment 16

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e22dff1f7912
https://hg.mozilla.org/mozilla-central/rev/9bf5112d0dbf
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55

Updated

2 years ago
Depends on: 1344319
You need to log in before you can comment on or make changes to this bug.