Status

defect
P2
normal
RESOLVED FIXED
3 years ago
Last year

People

(Reporter: linclark, Assigned: linclark)

Tracking

Trunk
Firefox 48
Dependency tree / graph

Firefox Tracking Flags

(firefox47 affected, firefox48 fixed)

Details

(Whiteboard: [btpp-fix-later])

Attachments

(1 attachment, 5 obsolete attachments)

Assignee

Description

3 years ago
If we end up using Immutable, we'll need to add it to our shared vendor dir.
Assignee

Comment 1

3 years ago
Assignee: nobody → lclark
Status: NEW → ASSIGNED
Assignee

Updated

3 years ago
Blocks: 1253376, 1253375
Assignee

Comment 2

3 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

3 years ago
Summary: [Immutable.js experiment] Add Immutable.js → Add Immutable.js
Assignee

Comment 3

3 years ago
Sorry, wrong patch!
Attachment #8730211 - Attachment is obsolete: true
Attachment #8730211 - Flags: review?(bgrinstead)
Attachment #8730220 - Flags: review?(bgrinstead)
Assignee

Comment 4

3 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

3 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

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

Updated

3 years ago
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.
Assignee

Comment 9

3 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)
(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

3 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

3 years ago
Keywords: checkin-needed

Comment 13

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a1f984af914e
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
See Also: → 1310573

Updated

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