1419407 - Remove Immutable usage in prefs reducer

Status: RESOLVED FIXED

(DevTools :: Console, enhancement, P3)

Description

[Nicolas Chevobbe [:nchevobbe]]

Immutable is adding some overhead to reducer code, and we can remove it.
This bug is about removing it from the prefs reducer : https://searchfox.org/mozilla-central/rev/919dce54f43356c22d6ff6b81c07ef412b1bf933/devtools/client/webconsole/new-console-output/reducers/prefs.js#9

We shouldn't use a Immutable.Record, but a plain object instead.

This might change how we get some properties or assertions in tests.
Make sure the following command does not return tests failure: 
`cd devtools/client/webconsole/ && npm install && npm test`

Comment 1

[Russell]

I'd like to try to work on this bug (my first!).

Comment 2

[Russell]

> We shouldn't use a Immutable.Record, but a plain object instead.

I'm curious what the preference would be: an exported constructor to replace the Immutable.Record constructor that's being used -- versus creating PrefState instances elsewhere as simple object literals?

Comment 3

[Nicolas Chevobbe [:nchevobbe]]

Hello Russel, 
Thanks for working on this :)

So the idea here is to replace the Immutable constructor by a simple function that returns a frozen object : 

```
const PrefState = (overrides) => Object.freeze(Object.assign({
  logLimit: 1000
}, overrides));
```

and simply call it from the reducer (state = PrefState())

We freeze it because redux expect us to return a new copy of the state, so we don't want to mutate the old state. If we do so here, freeze will throw, so it kind of protect us.

And we do set an `overrides` because we do create the state from the store as well : https://searchfox.org/mozilla-central/rev/33c90c196bc405e628bc868a4f4ba29b992478c0/devtools/client/webconsole/new-console-output/store.js#46 and here we force the logLimit (we use it for some test). So in store js, we only need to get rid of the `new` part.

Comment 4

[Russell]

Attachment: Bug 1419407 - remove Immutable usage in prefs reducer;

Review commit: https://reviewboard.mozilla.org/r/202560/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/202560/

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8931418 [details]
Bug 1419407 - remove Immutable usage in prefs reducer;

https://reviewboard.mozilla.org/r/202560/#review207906

::: commit-message-b6bed:1
(Diff revision 1)
> +Bug 1419407 - remove Immutable usage in prefs reducer

we miss the `;r=nchevobbe` part here :)

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

Things are looking good :) Let's push to try to see if everything works as expected.
In the meantime, can you amend your commit message and push to review again ? Thanks !

Comment 7

[Russell]

Comment on attachment 8931418 [details]
Bug 1419407 - remove Immutable usage in prefs reducer;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/202560/diff/1-2/

Comment 8

[Russell]

Done, sorry wasn't sure who would be reviewing :)

Comment 9

[Nicolas Chevobbe [:nchevobbe]]

TY is green , let's land this :)

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/df5b2fa427f7
remove Immutable usage in prefs reducer; r=nchevobbe

Comment 11

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/df5b2fa427f7