Do not load ImmutableJS library in the Net panel iframe

RESOLVED FIXED in Firefox 59

Status

enhancement
P2
normal
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: Honza, Assigned: msegreto, Mentored)

Tracking

(Blocks 1 bug)

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

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

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

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

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

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

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

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

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

a year ago
Attachment #8940067 - Attachment is obsolete: true
(Assignee)

Comment 14

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

a year ago
Keywords: checkin-needed

Comment 16

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

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

Updated

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