1418969 - Remove helper API from requests reducer

Status: RESOLVED FIXED

(DevTools :: Netmonitor, enhancement, P2)

Description

[Jan Honza Odvarko [:Honza] (always need-info? me)]

This is a follow-up for bug 1408182

We should remove helper API (relic from times when we used ImmutableJS) from
devtools/client/netmonitor/src/reducers:

* isEmpty 
* valueSeq

Look at the bottom of the file. The following function:

function mapNew(map) {
  let newMap = new Map(map);
  newMap.isEmpty = () => newMap.size == 0;
  newMap.valueSeq = () => [...newMap.values()];
  return newMap;
}

... should be as follows:

function mapNew(map) {
  return new Map(map);
}

... and consequently, it can be removed entirely and `new Map(map)` used directly.

The rest of the code base should be adopted not to use the API anymore.

Honza

Comment 1

[Jan Honza Odvarko [:Honza] (always need-info? me)]

> We should remove helper API (relic from times when we used ImmutableJS) from
> devtools/client/netmonitor/src/reducers:

Correction, the name of the file is:
devtools/client/netmonitor/src/reducers/requests.js

Honza

Comment 2

[Jan Honza Odvarko [:Honza] (always need-info? me)]

We should also remove helpers from `getSortedRequests` and `getDisplayedRequests` selectors.

devtools/client/netmonitor/src/selectors/requests.js

There are:
`get` and `isEmpty` API defined inside these two selectors -> remove it.

Honza

Comment 3

[Joshua Longhi]

Hey I'd like to fix this if that's possible. Thanks

Comment 4

[Jaewoongyu]

I wonder if I could work on this bug. Thank you

Comment 5

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Joshua Longhi from comment #3)
> Hey I'd like to fix this if that's possible. Thanks
Sure, assigned to you.


(In reply to Jaewoongyu from comment #4)
> I wonder if I could work on this bug. Thank you
Sorry, Joshua was first.

Honza

Comment 6

[Joshua Longhi]

What about 'requests: mapNew()' on line 31? Can this be changed to new Map()?

Comment 7

[Joshua Longhi]

Attachment: Remove helper API from requests reducer

Comment 8

[Ricky Chien [:rickychien] (inactive)]

Hey Joshua,

remember to add bug number & reviewer in your commit message `Bug 1418969 - Remove helper API from requests reducer r=honza`, so that honza will be informed about your review request. Otherwise, he might forgot your patch.

Comment 9

[Abhinav Koppula]

Honza, Can I take this forward?

Comment 10

[Abhinav Koppula]

Let me know only if no one is actively working on this issue and is free to be taken.

Comment 11

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8935966 [details] [diff] [review]
Remove helper API from requests reducer

Review of attachment 8935966 [details] [diff] [review]:
-----------------------------------------------------------------

This is good start, but other places in the code needs to be fixed as well.

Failures in this try-push should indicate what places in the code base should be fixed:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d67f1208ebe040adfe21a18747bc2de45995007

Honza

Comment 12

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Abhinav Koppula from comment #10)
> Let me know only if no one is actively working on this issue and is free to
> be taken.

Yes, it's free. Joshua doesn't seem to work on this one anymore.
Assigned to you Abhinav!

Honza

Comment 13

[Abhinav Koppula]

Attachment: Bug 1418969 - Remove helper API from requests reducer;

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

Comment 14

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8944188 [details]
Bug 1418969 - Remove helper API from requests reducer;

https://reviewboard.mozilla.org/r/214474/#review220156

Thanks Abhinav for working on this!

One inline comment + let's wait for try results.

Honza

::: devtools/client/netmonitor/src/reducers/requests.js:177
(Diff revision 1)
>        if (!action.open) {
>          nextState.selectedId = null;
>          return nextState;
>        }
>  
> -      if (!state.selectedId && !state.requests.isEmpty()) {
> +      if (!state.selectedId && !(state.requests.size == 0)) {

Please change to: state.requests.size > 0

Comment 15

[Abhinav Koppula]

Comment on attachment 8944188 [details]
Bug 1418969 - Remove helper API from requests reducer;

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

Comment 16

[Abhinav Koppula]

Comment on attachment 8944188 [details]
Bug 1418969 - Remove helper API from requests reducer;

https://reviewboard.mozilla.org/r/214474/#review220156

Done.

Comment 17

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8944188 [details]
Bug 1418969 - Remove helper API from requests reducer;

https://reviewboard.mozilla.org/r/214474/#review220498

Looks good!

R+ (try green)

Honza

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/df6485ef321a
Remove helper API from requests reducer; r=Honza

Comment 19

[Andrei Ciure[:aciure]]

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