Closed Bug 1253784 Opened 4 years ago Closed 4 years ago

Add Immutable.js

Categories

(DevTools :: Shared Components, defect, P2)

defect

Tracking

(firefox47 affected, firefox48 fixed)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox47 --- affected
firefox48 --- fixed

People

(Reporter: linclark, Assigned: linclark)

References

Details

(Whiteboard: [btpp-fix-later])

Attachments

(1 file, 5 obsolete files)

If we end up using Immutable, we'll need to add it to our shared vendor dir.
Attached patch Bug1253784-add-immutable.patch (obsolete) — Splinter Review
Assignee: nobody → lclark
Status: NEW → ASSIGNED
Blocks: 1253376, 1253375
Attached patch Bug1253784-add-immutable.patch (obsolete) — Splinter Review
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)
Summary: [Immutable.js experiment] Add Immutable.js → Add Immutable.js
Attached patch Bug1253784-add-immutable.patch (obsolete) — Splinter Review
Sorry, wrong patch!
Attachment #8730211 - Attachment is obsolete: true
Attachment #8730211 - Flags: review?(bgrinstead)
Attachment #8730220 - Flags: review?(bgrinstead)
Attached patch Bug1253784-add-immutable.patch (obsolete) — Splinter Review
Gah! did it again
Attachment #8730220 - Attachment is obsolete: true
Attachment #8730220 - Flags: review?(bgrinstead)
Attachment #8730222 - Flags: review?(bgrinstead)
Attached patch Bug1253784-add-immutable.patch (obsolete) — Splinter Review
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)
Priority: -- → P2
Whiteboard: [btpp-fix-later]
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)
Blocks: 1239436
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+
(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.
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)
(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).
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+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a1f984af914e
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
See Also: → 1310573
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.