Remove Immutable usage in filters reducer

RESOLVED FIXED in Firefox 59

Status

P3
normal
RESOLVED FIXED
a year ago
9 months ago

People

(Reporter: nchevobbe, Assigned: miller.time.baby, Mentored)

Tracking

(Blocks: 1 bug, {good-first-bug})

Trunk
Firefox 59
good-first-bug

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

a year ago
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`
(Reporter)

Updated

a year ago
Blocks: 1156747
(Reporter)

Updated

a year ago
Blocks: 1419407
(Reporter)

Updated

a year ago
Blocks: 1419410
(Reporter)

Updated

a year ago
No longer blocks: 1419407
(Reporter)

Updated

a year ago
No longer blocks: 1419410
(Reporter)

Updated

a year ago
Mentor: nchevobbe
Keywords: good-first-bug
Priority: -- → P3
(Assignee)

Comment 1

a year ago
I'd like to start working on this bug
(Reporter)

Comment 2

a year ago
Sure, thanks !
Assignee: nobody → miller.time.baby
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
(Reporter)

Comment 4

a year ago
mozreview-review
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
Attachment #8931445 - Flags: review?(nchevobbe) → review-
Comment hidden (mozreview-request)
(Reporter)

Comment 6

a year ago
mozreview-review
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.
Attachment #8931445 - Flags: review?(nchevobbe) → review+
Comment hidden (mozreview-request)
(Reporter)

Comment 8

a year ago
TRY is green, we can land this :)
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 hidden (mozreview-request)
(Assignee)

Comment 11

a year ago
ping review please
Flags: needinfo?(nchevobbe)
(Reporter)

Comment 12

a year ago
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
Flags: needinfo?(nchevobbe)
(Assignee)

Comment 13

a year ago
(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

a year ago
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1c1a7fcd5056
remove Immutable usage in filters reducer; r=nchevobbe

Comment 15

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1c1a7fcd5056
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox59: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59

Updated

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