The memory tool currently fakes immutability. https://dxr.mozilla.org/mozilla-central/source/devtools/client/memory/reducers/diffing.js#108 Let's see what this would look like if handled with Immutable.js
Assignee: nobody → lclark
Status: NEW → ASSIGNED
Created attachment 8727014 [details] [diff] [review] Bug1253376-immutable-memory-tool.patch This is a quick and dirty first pass at the diffing state. A couple of things are temporarily broken/inconsistent: - You'll need to apply the patch from Bug 1253784 first. - (Temporarily) I'm breaking snapshot census display. This is because I changed the census object to Immutable in the diffing reducer, but not the snapshot. If you continue with a diff, you'll see that it works. - (Temporarily) Some of the deeper nested objects are still mutable. For discussion: - How do folks feel about having to use .get() (in components, etc)? I know some teams are converting to regular objects in selectors via reselect. - Do we want to make the whole state tree immutable? If we do, we'll need to use something like redux-immutable. - Are there any other questions we want to answer before moving forward with Immutable?
I'd be interested to see that with Immutable.Record  in place of Immutable.Map. My guess is that the patch would be considerably smaller, and more natural. The downside of using Record is the need to define your records, but if we were to keep all of our Record definitions in the same place then there would be a nice self documentation effect. : https://facebook.github.io/immutable-js/docs/#/Record
Created attachment 8728373 [details] [diff] [review] Bug1253376-immutable-memory-tool.patch You're right, that is waaaaay smaller and more natural. It should also remove any need for Redux Immutable, I think. New patch, but it's still quick and dirty so previous provisos apply.
Attachment #8727014 - Attachment is obsolete: true
Created attachment 8728672 [details] [diff] [review] Bug1253376-immutable-memory-tool.patch This patch uses setIn(), deleteIn(), etc to simplify the logic.
Attachment #8728373 - Attachment is obsolete: true
A couple things: * The memory tool doesn't "fake" immutability, for the most part. Almost all state data is actually immutable, as it is Object.freeze()'d and updates happen via `immutableUpdate`. The exception is mostly data received directly via structured cloning from the HeapAnalysisWorker rather than generated by the frontend itself when reducing actions. I don't think it makes sense to walk this data and convert it to Immutable structures or use Immutable.js in the worker. * I'm wary of going all-in on Immutable.js for all-the-things right off the bat. Such a large scale change calls for closer evaluation, and I just haven't done that yet. I remain tentatively optimistic, however. * A great place to start would be with immutable sets, which Immutable.js has as (you guessed it) `Immutable.Set`. Right now, we mutate native `Set` objects, but update a wrapper object when we do so that we can maintain referential transparency. This is the closest we come to faking immutability. This would be a great piece of cruft to remove and replace with a proper immutable set. https://dxr.mozilla.org/mozilla-central/search?q=%22warning%3A+mutable+operations%22+path%3Adevtools%2Fclient%2Fmemory%2Freducers&redirect=true&case=false
Err I meant "maintain referential *equality*", and specifically inside `shouldComponentUpdate` component methods.
Right, if you look in the patch you'll see that it does use Immutable.Set, very specifically in that great place to start that you mention.
Priority: -- → P2
I added a patch replace the cruft that Nick referred to in Comment 5. See Bug 1253784
See Also: → bug 1239436
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.