1419405 - Remove Immutable usage in filters 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 filters reducer : https://searchfox.org/mozilla-central/rev/919dce54f43356c22d6ff6b81c07ef412b1bf933/devtools/client/webconsole/new-console-output/reducers/filters.js#8,11

We shouldn't use a Immutable.Record, but a plain object instead, and in reducers, we can use Object.assign() or the spread operator to return a new copy of the state.

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 start working on this bug

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

Sure, thanks !

Comment 3

[Russell]

Attachment: Bug 1419405 - remove Immutable usage in filters reducer;

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

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8931445 [details]
Bug 1419405 - remove Immutable usage in filters reducer;

https://reviewboard.mozilla.org/r/202596/#review208142

Thanks Russel, this looks good !
I only have one comment regarding doing unnecessary work.

::: devtools/client/webconsole/new-console-output/reducers/filters.js:15
(Diff revision 1)
> -const FilterState = Immutable.Record(constants.DEFAULT_FILTERS_VALUES);
> +const FilterState = (overrides) => Object.freeze(
> +  Object.assign({}, constants.DEFAULT_FILTERS_VALUES, overrides)
> +);
>  
> -function filters(state = new FilterState(), action) {
> +function filters(state = FilterState(), action) {
> +  const newState = Object.assign({}, state);

we may not want to do this here : This function is called every time an action is dispatched (not only a "filter" one, it could be when we add message for example).
So, here we are doing extra work that might not be useful.

We can still have a convenient `cloneState` function if you want : 

function cloneState(state, override) {
  return Object.assign({}, state, override);
}

and use it where it make sense : 

case constants.FILTER_TOGGLE:
  const active = !state[filter];
  return cloneState(state, {[filter]: active})
  
Or we can use Object.assign inline, your call

Comment 5

[Russell]

Comment on attachment 8931445 [details]
Bug 1419405 - remove Immutable usage in filters reducer;

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

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8931445 [details]
Bug 1419405 - remove Immutable usage in filters reducer;

https://reviewboard.mozilla.org/r/202596/#review208182

Looks good to me, thanks a lot !
mocha tests are green and manual testing did not reveal anything wrong. Let's wait TRY results before landing.

Comment 7

[Russell]

Comment on attachment 8931445 [details]
Bug 1419405 - remove Immutable usage in filters reducer;

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

Comment 8

[Nicolas Chevobbe [:nchevobbe]]

TRY is green, we can land this :)

Comment 9

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 454c842d00b7 -d c5d69465077c: rebasing 435748:454c842d00b7 "Bug 1419405 - remove Immutable usage in filters reducer; r=nchevobbe" (tip)
merging devtools/client/webconsole/new-console-output/store.js
warning: conflicts while merging devtools/client/webconsole/new-console-output/store.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 10

[Russell]

Comment on attachment 8931445 [details]
Bug 1419405 - remove Immutable usage in filters reducer;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/202596/diff/3-4/

Comment 11

[Russell]

ping review please

Comment 12

[Nicolas Chevobbe [:nchevobbe]]

Oh, sorry Russel, it wasn't in my queue.
When an already r+ patch need another review, you need to go to the detail of the attachment on bugzilla, and put the `r` flag from `+` to `?`.
This will ping the person again, and it will show up in their review queue.

I'm looking at the review again

Comment 13

[Russell]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #12)
> Oh, sorry Russel, it wasn't in my queue.
> When an already r+ patch need another review, you need to go to the detail
> of the attachment on bugzilla, and put the `r` flag from `+` to `?`.
> This will ping the person again, and it will show up in their review queue.
> 
> I'm looking at the review again

Oh my bad, I didn't know :(

Comment 14

[Pulsebot]

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

Comment 15

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/1c1a7fcd5056