Closed Bug 1399390 Opened 2 years ago Closed 2 years ago

Net monitor broken when running on top of the Launchpad again

Categories

(DevTools :: Netmonitor, enhancement, P1)

enhancement

Tracking

(firefox57 fixed)

RESOLVED FIXED
Firefox 57
Tracking Status
firefox57 --- fixed

People

(Reporter: Honza, Assigned: Honza)

References

Details

Attachments

(2 files)

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
We need automated tests since it's currently simple to break WebPack bundling.
Should be done in separate bug.

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

Honza
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)
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 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)
(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 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 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)
(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
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
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.