Closed
Bug 1253784
Opened 8 years ago
Closed 8 years ago
Add Immutable.js
Categories
(DevTools :: Shared Components, defect, P2)
DevTools
Shared Components
Tracking
(firefox47 affected, firefox48 fixed)
RESOLVED
FIXED
Firefox 48
People
(Reporter: linclark, Assigned: linclark)
References
Details
(Whiteboard: [btpp-fix-later])
Attachments
(1 file, 5 obsolete files)
151.74 KB,
patch
|
linclark
:
review+
|
Details | Diff | Splinter Review |
If we end up using Immutable, we'll need to add it to our shared vendor dir.
Assignee | ||
Comment 1•8 years ago
|
||
Assignee: nobody → lclark
Status: NEW → ASSIGNED
Assignee | ||
Updated•8 years ago
|
Assignee | ||
Comment 2•8 years ago
|
||
I added the immutable.min.js as immutable.js. Let me know if I should change that around. Do you know who I should flag for the license review?
Attachment #8727005 -
Attachment is obsolete: true
Attachment #8730211 -
Flags: review?(bgrinstead)
Assignee | ||
Updated•8 years ago
|
Summary: [Immutable.js experiment] Add Immutable.js → Add Immutable.js
Assignee | ||
Comment 3•8 years ago
|
||
Sorry, wrong patch!
Attachment #8730211 -
Attachment is obsolete: true
Attachment #8730211 -
Flags: review?(bgrinstead)
Attachment #8730220 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 4•8 years ago
|
||
Gah! did it again
Attachment #8730220 -
Attachment is obsolete: true
Attachment #8730220 -
Flags: review?(bgrinstead)
Attachment #8730222 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 5•8 years ago
|
||
This is why I should not talk and post patches at the same time
Attachment #8730222 -
Attachment is obsolete: true
Attachment #8730222 -
Flags: review?(bgrinstead)
Attachment #8730232 -
Flags: review?(bgrinstead)
Assignee | ||
Updated•8 years ago
|
Priority: -- → P2
Whiteboard: [btpp-fix-later]
Comment 6•8 years ago
|
||
Comment on attachment 8730232 [details] [diff] [review] Bug1253784-add-immutable.patch Review of attachment 8730232 [details] [diff] [review]: ----------------------------------------------------------------- > Do you know who I should flag for the license review? Forwarding license review to Gerv. Gerv, one other question I have - should we also add a text file with the license [0] in the directory with the source file or is adding to about:license enough? It looks like we've been including it in both places previously in this vendor/ directory but maybe it's not needed. [0]: https://github.com/facebook/immutable-js/blob/master/LICENSE
Attachment #8730232 -
Flags: review?(bgrinstead) → review?(gerv)
Comment 7•8 years ago
|
||
Comment on attachment 8730232 [details] [diff] [review] Bug1253784-add-immutable.patch Review of attachment 8730232 [details] [diff] [review]: ----------------------------------------------------------------- r=gerv with these two changes. Gerv ::: devtools/client/shared/vendor/immutable.js @@ +1,2 @@ > +/** > + * Copyright (c) 2014-2015, Facebook, Inc. Please don't check obfuscated code into the tree without an unobfuscated version. The MIT license doesn't require that we provide readable source code, but it's good practise. ::: toolkit/content/license.html @@ +3463,5 @@ > +<pre> > +BSD License > + > +For Immutable JS software > + Please remove the four lines immediately above this comment.
Attachment #8730232 -
Flags: review?(gerv) → review+
Comment 8•8 years ago
|
||
(In reply to Gervase Markham [:gerv] from comment #7) > ::: devtools/client/shared/vendor/immutable.js > @@ +1,2 @@ > > +/** > > + * Copyright (c) 2014-2015, Facebook, Inc. > > Please don't check obfuscated code into the tree without an unobfuscated > version. The MIT license doesn't require that we provide readable source > code, but it's good practise. Additionally, it would sure be nice for debugging if the devtools loader required a non-minified build when DEBUG_JS_MODULES was set. We do something similar for React already. Not a blocker for landing initially, but we should probably do it in a follow up as we start actually using Immutable.js.
Assignee | ||
Comment 9•8 years ago
|
||
I wanted to ask about that. Are we actually using any minified versions? It seems like the react we use in prod isn't minified... it just doesn't contain the prop validation stuff. If we aren't using any minified libs at this point, maybe this should just include the non-minified version. Then we could possibly add minified versions of all the libs in a different issue. Would that be ok?
Flags: needinfo?(bgrinstead)
Comment 10•8 years ago
|
||
(In reply to Lin Clark [:linclark] from comment #9) > I wanted to ask about that. Are we actually using any minified versions? It > seems like the react we use in prod isn't minified... it just doesn't > contain the prop validation stuff. If we aren't using any minified libs at > this point, maybe this should just include the non-minified version. Then we > could possibly add minified versions of all the libs in a different issue. > Would that be ok? Does the non-minified immutable.js have debug assertions that get stripped in minification? If not, then using it all the time sounds good to me. Ideally we would always use a non-minified version, and if there is a non-minified version that also has extra debug checks, then we would use that when DEBUG_JS_MODULES (rather than the non-minified version without the extra debug checks).
Assignee | ||
Comment 11•8 years ago
|
||
This patch adds the unminified version and removes the comment that :gerv mentioned.
Attachment #8730232 -
Attachment is obsolete: true
Flags: needinfo?(bgrinstead)
Attachment #8733460 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 12•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/a1f984af914e
Keywords: checkin-needed
Comment 13•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a1f984af914e
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•