1419410 - Remove Immutable usage in ui 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 ui reducer : https://searchfox.org/mozilla-central/rev/c633ffa4c4611f202ca11270dcddb7b29edddff8/devtools/client/webconsole/new-console-output/reducers/ui.js#15,21-27

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

[Sudheesh Singanamalla (:ShellHacker)]

Hi Nicolas,

From what I understand, Do we have to change the Immutable record to an Object and then have `state = Object.assign({}, UiState)`.

However, this breaks the test cases, Are there specific test cases that I'd have to look at to fix the assertions? There are too many usages of `UiState` and I seem to be confused.

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

Hello Sudheesh, 

You are right, we need to not use a Immutable.Record, but a simple object, and switch all the state.set to Object.assign.
Then some tests might be failing if they try to access state properties like state.ui.get("filterBarVisible") where now we need to do state.ui.filterBarVisible because we have a plain object.

Can you paste here the test failures you were seeing ?

Also, you can come in Slack to chat if you want to https://devtools-html-slack.herokuapp.com/

Comment 3

[Sudheesh Singanamalla (:ShellHacker)]

Attachment: Initial-version-webconsole

Thank you for the invite. I am attaching the patch file I have so far and joining slack for further discussion.

The errors while running the tests are as follows

```
 150 passing (2s)
  1 pending
  3 failing

  1) ConsoleOutput component: Render every message when initialized:
     Error: Expected 39 to be 100
      at assert (c:\mozilla-source\mozilla-central\devtools\client\webconsole\node_modules\expect\lib\assert.js:29:9)
      at Expectation.toBe (c:\mozilla-source\mozilla-central\devtools\client\webconsole\node_modules\expect\lib\Expectation.js:66:28)
      at Context.<anonymous> (c:/mozilla-source/mozilla-central/devtools/client/webconsole/new-console-output/test/components/console-output.test.js:47:46)
      at callFn (c:\mozilla-source\mozilla-central\devtools\client\webconsole\node_modules\mocha\lib\runnable.js:326:21)
      at Test.Runnable.run (c:\mozilla-source\mozilla-central\devtools\client\webconsole\node_modules\mocha\lib\runnable.js:319:7)
      at Runner.runTest (c:\mozilla-source\mozilla-central\devtools\client\webconsole\node_modules\mocha\lib\runner.js:422:10)
      at c:\mozilla-source\mozilla-central\devtools\client\webconsole\node_modules\mocha\lib\runner.js:528:12
      at next (c:\mozilla-source\mozilla-central\devtools\client\webconsole\node_modules\mocha\lib\runner.js:342:14)
      at c:\mozilla-source\mozilla-central\devtools\client\webconsole\node_modules\mocha\lib\runner.js:352:7
      at next (c:\mozilla-source\mozilla-central\devtools\client\webconsole\node_modules\mocha\lib\runner.js:284:14)
      at Immediate.<anonymous> (c:\mozilla-source\mozilla-central\devtools\client\webconsole\node_modules\mocha\lib\runner.js:320:5)

  2) FilterBar component: displays filter bar when button is clicked:
     Error: Expected undefined to be true
      at assert (c:\mozilla-source\mozilla-central\devtools\client\webconsole\node_modules\expect\lib\assert.js:29:9)
      at Expectation.toBe (c:\mozilla-source\mozilla-central\devtools\client\webconsole\node_modules\expect\lib\Expectation.js:66:28)
      at Context.<anonymous> (c:/mozilla-source/mozilla-central/devtools/client/webconsole/new-console-output/test/components/filter-bar.test.js:205:57)
      at callFn (c:\mozilla-source\mozilla-central\devtools\client\webconsole\node_modules\mocha\lib\runnable.js:326:21)
      at Test.Runnable.run (c:\mozilla-source\mozilla-central\devtools\client\webconsole\node_modules\mocha\lib\runnable.js:319:7)
      at Runner.runTest (c:\mozilla-source\mozilla-central\devtools\client\webconsole\node_modules\mocha\lib\runner.js:422:10)
      at c:\mozilla-source\mozilla-central\devtools\client\webconsole\node_modules\mocha\lib\runner.js:528:12
      at next (c:\mozilla-source\mozilla-central\devtools\client\webconsole\node_modules\mocha\lib\runner.js:342:14)
      at c:\mozilla-source\mozilla-central\devtools\client\webconsole\node_modules\mocha\lib\runner.js:352:7
      at next (c:\mozilla-source\mozilla-central\devtools\client\webconsole\node_modules\mocha\lib\runner.js:284:14)
      at Immediate.<anonymous> (c:\mozilla-source\mozilla-central\devtools\client\webconsole\node_modules\mocha\lib\runner.js:320:5)

  3) FilterBar component: toggles persist logs when checkbox is clicked:
     Error: Expected undefined to be true
      at assert (c:\mozilla-source\mozilla-central\devtools\client\webconsole\node_modules\expect\lib\assert.js:29:9)
      at Expectation.toBe (c:\mozilla-source\mozilla-central\devtools\client\webconsole\node_modules\expect\lib\Expectation.js:66:28)
      at Context.<anonymous> (c:/mozilla-source/mozilla-central/devtools/client/webconsole/new-console-output/test/components/filter-bar.test.js:267:52)
      at callFn (c:\mozilla-source\mozilla-central\devtools\client\webconsole\node_modules\mocha\lib\runnable.js:326:21)
      at Test.Runnable.run (c:\mozilla-source\mozilla-central\devtools\client\webconsole\node_modules\mocha\lib\runnable.js:319:7)
      at Runner.runTest (c:\mozilla-source\mozilla-central\devtools\client\webconsole\node_modules\mocha\lib\runner.js:422:10)
      at c:\mozilla-source\mozilla-central\devtools\client\webconsole\node_modules\mocha\lib\runner.js:528:12
      at next (c:\mozilla-source\mozilla-central\devtools\client\webconsole\node_modules\mocha\lib\runner.js:342:14)
      at c:\mozilla-source\mozilla-central\devtools\client\webconsole\node_modules\mocha\lib\runner.js:352:7
      at next (c:\mozilla-source\mozilla-central\devtools\client\webconsole\node_modules\mocha\lib\runner.js:284:14)
      at Immediate.<anonymous> (c:\mozilla-source\mozilla-central\devtools\client\webconsole\node_modules\mocha\lib\runner.js:320:5)

```

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8930851 [details] [diff] [review]
Initial-version-webconsole

Discussed on Slack on how to proceed

Comment 5

[Sudheesh Singanamalla (:ShellHacker)]

Attachment: Bug 1419410 Removes Immutable usage in UI reducer;

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

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8931209 [details]
Bug 1419410 Removes Immutable usage in UI reducer;

https://reviewboard.mozilla.org/r/202306/#review207722

I have a few comments, but the reducer code itself looks perfectly valid, and the tests are all green :)

::: devtools/client/webconsole/new-console-output/reducers/ui.js:15
(Diff revision 1)
>    INITIALIZE,
>    PERSIST_TOGGLE,
>    SELECT_NETWORK_MESSAGE_TAB,
>    TIMESTAMPS_TOGGLE,
>  } = require("devtools/client/webconsole/new-console-output/constants");
> -const Immutable = require("devtools/client/shared/vendor/immutable");
> +// const Immutable = require("devtools/client/shared/vendor/immutable");

we can delete the line completely

::: devtools/client/webconsole/new-console-output/reducers/ui.js:21
(Diff revision 1)
> -const UiState = Immutable.Record({
> +const UiState = () => Object.freeze({
>    filterBarVisible: false,
>    initialized: false,
>    networkMessageActiveTabId: PANELS.HEADERS,
>    persistLogs: false,
>    timestampsVisible: true,
>  });

As you may have seen, in the store, we create the UiState with some default values, so we need to be able to handle them here: 

```
const UiState = (overrides) => Object.freeze(Object.assign({
  filterBarVisible: false,
  initialized: false,
  networkMessageActiveTabId: PANELS.HEADERS,
  persistLogs: false,
  timestampsVisible: true,
}, overrides));
```

::: devtools/client/webconsole/new-console-output/store.js:57
(Diff revision 1)
> -    ui: new UiState({
> +    ui: Object.assign({}, {
>        filterBarVisible: Services.prefs.getBoolPref(PREFS.UI.FILTER_BAR),
>        networkMessageActiveTabId: "headers",
>        persistLogs: Services.prefs.getBoolPref(PREFS.UI.PERSIST),
>      })

we should re-use the UiState we define in the reducer : 
```
ui: UiState({
  …
})
```

Comment 7

[Sudheesh Singanamalla (:ShellHacker)]

Comment on attachment 8931209 [details]
Bug 1419410 Removes Immutable usage in UI reducer;

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

Comment 8

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8931209 [details]
Bug 1419410 Removes Immutable usage in UI reducer;

https://reviewboard.mozilla.org/r/202306/#review207734

::: devtools/client/webconsole/new-console-output/reducers/ui.js:31
(Diff revision 2)
> -      return state.set("filterBarVisible", !state.filterBarVisible);
> +      return Object.assign({}, state, {filterBarVisible : !state.filterBarVisible});
>      case PERSIST_TOGGLE:
> -      return state.set("persistLogs", !state.persistLogs);
> +      return Object.assign({}, state, {persistLogs : !state.persistLogs});
>      case TIMESTAMPS_TOGGLE:
> -      return state.set("timestampsVisible", action.visible);
> +      return Object.assign({}, state, {timestampsVisible : action.visible});
>      case SELECT_NETWORK_MESSAGE_TAB:
> -      return state.set("networkMessageActiveTabId", action.id);
> +      return Object.assign({}, state, {networkMessageActiveTabId : action.id});
>      case INITIALIZE:
> -      return state.set("initialized", true);
> +      return Object.assign({}, state, {initialized : true});

one last bit, I think there will be failures because of the space before `:`

Object should be
`{filterBarVisible: !state.filterBarVisible});`
and not
`{filterBarVisible : !state.filterBarVisible});`

If you did not do it yet, you can have a look at http://docs.firefox-dev.tools/contributing/eslint.html and see how to set eslint up.

Comment 9

[Sudheesh Singanamalla (:ShellHacker)]

Comment on attachment 8931209 [details]
Bug 1419410 Removes Immutable usage in UI reducer;

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

Comment 10

[Nicolas Chevobbe [:nchevobbe]]

All looks good now, let's land this

Comment 11

[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 f15e9b1f7539 -d a2b9ac473749: rebasing 435512:f15e9b1f7539 "Bug 1419410 Removes Immutable usage in UI reducer; r=nchevobbe" (tip)
merging devtools/client/webconsole/new-console-output/actions/ui.js
merging devtools/client/webconsole/new-console-output/reducers/ui.js
merging devtools/client/webconsole/new-console-output/store.js
warning: conflicts while merging devtools/client/webconsole/new-console-output/reducers/ui.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 12

[Sudheesh Singanamalla (:ShellHacker)]

Comment on attachment 8931209 [details]
Bug 1419410 Removes Immutable usage in UI reducer;

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

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/982a35df0c88
Removes Immutable usage in UI reducer; r=nchevobbe

Comment 14

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/982a35df0c88