Closed Bug 1419370 Opened 7 years ago Closed 6 years ago

Do not load ImmutableJS library in the Net panel iframe

Categories

(DevTools :: Netmonitor, enhancement, P2)

Product:
DevTools
Component:
Netmonitor
Type:
enhancement

Tracking

(firefox59 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: Honza, Assigned: msegreto, Mentored)

References

Details

(Whiteboard: good-first-bug)

Attachments

(1 file, 3 obsolete files)

remove immutablejs from netmonitor
3.67 KB, patch
Honza
: review+
Details | Diff | Splinter Review 
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
Depends on: 1419366, 1419367, 1419368, 1419369
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
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"
```
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.
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)
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`.
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
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)
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
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+
Attachment #8940067 - Attachment is obsolete: true
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
Keywords: checkin-needed
Pushed by ncsoregi@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6984e56259d8
remove immutablejs from netmonitor. r=honza
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6984e56259d8
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: