Do not load ImmutableJS library in the Net panel iframe

RESOLVED FIXED in Firefox 59

Status

enhancement
P2
normal
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: Honza, Assigned: msegreto, Mentored)

Tracking

unspecified
Firefox 59
Dependency tree / graph

Firefox Tracking Flags

(firefox59 fixed)

Details

(Whiteboard: good-first-bug)

Attachments

(1 attachment, 3 obsolete attachments)

As soon as ImmutableJS is removed from NetPanel entirely we can stop loading the library in the iframe (and make the panel opening faster).

All the following bugs needs to be fixed first:
* Bug 1419366 - NetMonitor: Stop using ImmutableJS in filters reducer
* Bug 1419367 - NetMonitor: Stop using ImmutableJS in sort reducer
* Bug 1419368 - NetMonitor: Stop using ImmutableJS in timing-markers reducer
* Bug 1419369 - NetMonitor: Stop using ImmutableJS in ui reducer

Honza
Mentor: odvarko
Priority: -- → P2
Whiteboard: good-first-bug
Summary: Do not load ImmutableJS library int the Net panel iframe → Do not load ImmutableJS library in the Net panel iframe
Assignee

Comment 1

2 years ago
Remove the use of `Immutable.is` in Toolbar, this method should
map directly to `Object.is`. The props being passed to `is` were
already plain JS objects from what I understand.

Yarn was used to remove the Immutable JS package and webpack was
configured to not include webpack from shared assets.
Marco, thanks for the patch! 
We just come back from the year-end vacation today, will review it soon
Assignee: nobody → msegreto
Status: NEW → ASSIGNED
Comment on attachment 8938907 [details] [diff] [review]
remove immutablejs from netmonitor

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

::: devtools/client/netmonitor/yarn.lock
@@ -2415,4 @@
>    version "3.0.6"
>    resolved "https://registry.yarnpkg.com/immediate/-/immediate-3.0.6.tgz#9db1dbd0faf8de6fbe0f5dd5e56bb606280de69b"
>  
> -immutable@^3.7.6, immutable@^3.8.1:

this section are all related to immutable js, we can remove this section entirely

```
immutable@^3.7.6, immutable@^3.8.1:
  version "3.8.1"
  resolved "https://registry.yarnpkg.com/immutable/-/immutable-3.8.1.tgz#200807f11ab0f72710ea485542de088075f68cd2"
```
Assignee

Comment 4

2 years ago
I'll take a look tonight. I thought yarn would've removed it from the lockfile when I rand the remove command, but maybe there's another command I have to run.
Assignee

Comment 5

2 years ago
Remove the use of `Immutable.is` in Toolbar, this method should
map directly to `Object.is`. The props being passed to `is` were
already plain JS objects from what I understand.

Yarn was used to remove the Immutable JS package and webpack was
configured to not include webpack from shared assets.
***

Removed Immutable reference in yarn.lock
Thanks for the update!

I found devtools-launchpad has dependency with immutablejs, 
https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/yarn.lock#1589

Instead of manually remove the data in `yarn.lock`, maybe you can run `yarn install` and submit the result instead.


When you submit a new patch, Please go to detail > review and set :honza as reviewer since he reviewed most of immutablejs related code, thanks.
Flags: needinfo?(msegreto)
Assignee

Comment 7

2 years ago
Remove the use of `Immutable.is` in Toolbar, this method should
map directly to `Object.is`. The props being passed to `is` were
already plain JS objects from what I understand.

Yarn was used to remove the Immutable JS package and webpack was
configured to not include webpack from shared assets.
***

Removed Immutable reference in yarn.lock
***
Generated yarn.lock file

By running `yarn install`.
Assignee

Updated

2 years ago
Attachment #8940067 - Flags: review?(odvarko)
Attachment #8938907 - Attachment is obsolete: true
Attachment #8939719 - Attachment is obsolete: true
Comment on attachment 8940067 [details] [diff] [review]
remove immutablejs from netmonitor

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

Thanks for working on this!

Please yet update netmonitor/README.md

We use [immutable.js](http://facebook.github.io/immutable-js/) and [reselect](https://github.com/reactjs/reselect) libraries to return a new state object efficiently.

to something like:

We use [reselect](https://github.com/reactjs/reselect) library to return a access state object efficiently. We don't use [immutable.js](http://facebook.github.io/immutable-js/) due to performance issues.

(there is also typo in the README.md file that might be fixed too: infomation->information)

What about yarn.lock? I am still seeing immutable in there.

Honza
Attachment #8940067 - Flags: review?(odvarko)
Comment on attachment 8940067 [details] [diff] [review]
remove immutablejs from netmonitor

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

::: devtools/client/netmonitor/src/actions/requests.js
@@ +71,1 @@
>  

Please remove both console.logs
Assignee

Comment 10

2 years ago
So for `yarn.lock`, I think it's referencing Immutable in devtools-launchpad. I don't want to manually edit that file as it should be generated by yarn anyway. The current version of it was from running `yarn install`.
Flags: needinfo?(msegreto)
Assignee

Comment 11

2 years ago
Remove the use of `Immutable.is` in Toolbar, this method should
map directly to `Object.is`. The props being passed to `is` were
already plain JS objects from what I understand.

Yarn was used to remove the Immutable JS package and webpack was
configured to not include webpack from shared assets.
***

Removed Immutable reference in yarn.lock
***
Generated yarn.lock file

By running `yarn install`.
***
Remove debugging statements
Assignee

Comment 12

2 years ago
Comment on attachment 8940295 [details] [diff] [review]
remove immutablejs from netmonitor

Remove debugging console.log statements
Attachment #8940295 - Flags: review?(odvarko)
Comment on attachment 8940295 [details] [diff] [review]
remove immutablejs from netmonitor

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

Ok, looks good to me now, thanks!

R+

Please make sure the old patches are marked as obsolete (edit details of the patch to access the settings)

Honza
Attachment #8940295 - Flags: review?(odvarko) → review+
Assignee

Updated

Last year
Attachment #8940067 - Attachment is obsolete: true
Assignee

Comment 14

Last year
Can you push this to the try server to see that it passes all the tests? Thanks
(In reply to Marco Segreto from comment #14)
> Can you push this to the try server to see that it passes all the tests?
> Thanks
Please NI me the next time otherwise I can easily miss the request.

Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=17e32dfaf9f14542eed5ed24aebeda9aea16a2bf

Honza
Assignee

Updated

Last year
Keywords: checkin-needed

Comment 16

Last year
Pushed by ncsoregi@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6984e56259d8
remove immutablejs from netmonitor. r=honza
Keywords: checkin-needed

Comment 17

Last year
bugherder
https://hg.mozilla.org/mozilla-central/rev/6984e56259d8
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59

Updated

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