Land react-redux for developer tools

RESOLVED FIXED in Firefox 44

Status

defect
RESOLVED FIXED
4 years ago
Last year

People

(Reporter: jsantell, Assigned: jsantell)

Tracking

43 Branch
Firefox 44
Dependency tree / graph

Firefox Tracking Flags

(firefox44 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

We have react and redux in our tree, and this small library combines them splendidly.
Posted patch 1213138-reactredux.patch (obsolete) — Splinter Review
r? gerv for the library being added
f? james for the wrapper used to include it in our tools
Attachment #8671709 - Flags: review?(gerv)
Attachment #8671709 - Flags: feedback?(jlong)
Comment on attachment 8671709 [details] [diff] [review]
1213138-reactredux.patch

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

Looks good to me!

::: devtools/client/shared/vendor/REACT_REDUX_UPGRADING
@@ +5,5 @@
> +If upgrading react-redux, define the correct paths and replace the require statements
> +for the module.exports case with the correct paths.
> +
> +var REACT_PATH = "devtools/client/shared/vendor/react";
> +var REDUX_PATH = "devtools/client/shared/vendor/redux";

Why do you specify the paths in here? The ones they need to update are in react-redux.js right?
Attachment #8671709 - Flags: feedback?(jlong) → feedback+
Gerv, any chance you could look at this soon, or can someone else review licensing? We're dependent on this for the fall release in 3 weeks.
Flags: needinfo?(gerv)
Comment on attachment 8671709 [details] [diff] [review]
1213138-reactredux.patch

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

::: devtools/client/shared/vendor/REACT_REDUX_UPGRADING
@@ +5,5 @@
> +If upgrading react-redux, define the correct paths and replace the require statements
> +for the module.exports case with the correct paths.
> +
> +var REACT_PATH = "devtools/client/shared/vendor/react";
> +var REDUX_PATH = "devtools/client/shared/vendor/redux";

Correct, this just lists what those variables should be in react-redux -- would it make more sense to remove these?
That's fine. Land the license in the usual way in toolkit/content/license.html.

Gerv
Flags: needinfo?(gerv)
Made changes based on James' feedback to clarify the upgrade notes, to make it look less like JavaScript and just list the paths.

Thanks Gerv!
Attachment #8671709 - Attachment is obsolete: true
Attachment #8671709 - Flags: review?(gerv)
Attachment #8673779 - Flags: review+
Attachment #8673779 - Flags: feedback+
https://hg.mozilla.org/mozilla-central/rev/276bf7fc547d
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44

Updated

Last year
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.