Net monitor broken when running on top of the Launchpad again

RESOLVED FIXED in Firefox 57

Status

()

Firefox
Developer Tools: Netmonitor
P1
normal
RESOLVED FIXED
7 months ago
7 months ago

People

(Reporter: Honza, Assigned: Honza)

Tracking

unspecified
Firefox 57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Assignee)

Description

7 months ago
Bug 1396600 introduced lazy loading of modules:

For example (devtools/shared/css/color.js):

loader.lazyRequireGetter(this, "CSS_ANGLEUNIT",
  "devtools/shared/css/properties-db", true);

Webpack is not resolving such require and there is runtime error (displayed in the Console panel) complaining about undefined 'loader' object.

Honza
(Assignee)

Comment 1

7 months ago
We need automated tests since it's currently simple to break WebPack bundling.
Should be done in separate bug.

Honza
(Assignee)

Comment 2

7 months ago
Created attachment 8907485 [details] [diff] [review]
net-webpack-lazyloader-workaround.patch

Here is a workaround for anyone who want to run the Network panel with the Launchpad in the meantime.

Honza
(Assignee)

Comment 3

7 months ago
Perhaps we could change:

loader.lazyRequireGetter(this, "CSS_ANGLEUNIT",
  "devtools/shared/css/properties-db", true);

to:

const {CSS_ANGLEUNIT} = require("lazy-loader!devtools/shared/css/properties-db");

1) And implement the support in DevTools loader (similarly to how 'raw' prefix is done)
2) Plus, introduce simple 'lazy-loader' webpack loader (so webpack understands the prefix)

@Alex What do you think about #1?

Honza
Flags: needinfo?(poirot.alex)
I don't see how lazy-loader! would be implemented?

Can you do like inspector and have this rewrite?
http://searchfox.org/mozilla-central/source/devtools/client/inspector/webpack/rewrite-lazy-require.js
Flags: needinfo?(poirot.alex)
Comment hidden (mozreview-request)
(Assignee)

Updated

7 months ago
Assignee: nobody → odvarko
Status: NEW → ASSIGNED
Depends on: 1395834
Priority: -- → P1
Comment on attachment 8907617 [details]
Bug 1399390 - Use lazy-require in Netmonitor's webpack.config;

Sorry, I know nothing about webpack.
Attachment #8907617 - Flags: review?(poirot.alex) → review?(jdescottes)

Comment 7

7 months ago
mozreview-review
Comment on attachment 8907617 [details]
Bug 1399390 - Use lazy-require in Netmonitor's webpack.config;

https://reviewboard.mozilla.org/r/179300/#review184428

::: devtools/client/netmonitor/webpack.config.js:39
(Diff revision 1)
>           */
>          test: /\.js/,
>          loader: "rewrite-raw",
>        },
> +      {
> +        test: /client(\/|\\)inspector(\/|\\).*\.js$/,

you probably want netmonitor instead of inspector in the regexp here?
Attachment #8907617 - Flags: review?(jdescottes)
Comment hidden (mozreview-request)
(Assignee)

Comment 9

7 months ago
(In reply to Julian Descottes [:jdescottes] from comment #7)
> you probably want netmonitor instead of inspector in the regexp here?
yes, fixed.

(btw. Alex, I forwarded the review to :gl since the patch is changing bits from the Inspector dir)

Honza

Comment 10

7 months ago
mozreview-review
Comment on attachment 8907617 [details]
Bug 1399390 - Use lazy-require in Netmonitor's webpack.config;

https://reviewboard.mozilla.org/r/179300/#review184458

I can still review this. The inspector's webpack config is not working at the moment so feel free to go forward with your patch.
Attachment #8907617 - Flags: review+

Comment 11

7 months ago
mozreview-review
Comment on attachment 8907617 [details]
Bug 1399390 - Use lazy-require in Netmonitor's webpack.config;

https://reviewboard.mozilla.org/r/179300/#review184586

I think we should add a README to the /shared/webpack/ folder to explain what is in this folder, who consumes the modules and how it is used.
Attachment #8907617 - Flags: review?(gl)
Comment hidden (mozreview-request)
(Assignee)

Comment 13

7 months ago
(In reply to Julian Descottes [:jdescottes] from comment #10)
> Comment on attachment 8907617 [details]
> Bug 1399390 - Use lazy-require in Netmonitor's webpack.config;
> 
> https://reviewboard.mozilla.org/r/179300/#review184458
> 
> I can still review this. The inspector's webpack config is not working at
> the moment so feel free to go forward with your patch.
Thanks for the review Julian!

(In reply to Gabriel [:gl] (ΦωΦ) from comment #11)
> I think we should add a README to the /shared/webpack/ folder to explain
> what is in this folder, who consumes the modules and how it is used.
Done

Honza

Comment 14

7 months ago
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2194f20eac24
Use lazy-require in Netmonitor's webpack.config; r=jdescottes
https://hg.mozilla.org/mozilla-central/rev/2194f20eac24
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
You need to log in before you can comment on or make changes to this bug.