Status

defect
P1
normal
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: nchevobbe, Assigned: Honza)

Tracking

Trunk
Firefox 57
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(2 attachments)

An update in the reps bundle broke the netmonitor Launchpad (because of the exponential operator **).
devtools-launchpad started using Webpack 3+ and the Netmonitor should do it too.

The same work happens in the console in Bug 1390815
Assignee: nobody → odvarko
Status: NEW → ASSIGNED
Priority: -- → P1
Comment hidden (mozreview-request)
There is a problem with the rewrite-raw webpack loader. It breaks loading of `variables.css` modules in client/shared/themes.js module.

Look for this line:

const variableFileContents = require("raw!devtools/client/themes/variables.css");

It's expecting to return a string that is parsed in getThemeFile() function.

STR:

1) Run network panel in Launchpad
2) Click the Reload button
3) Try to select some request in the list
4) You should see an error in the Console panel

Honza
Comment hidden (mozreview-request)
I am using a new theme-loader as a workaround. But, if it looks good enough we can just update (raw!) tests and land.

Honza
One more note, I put 'webpack' into Netmonitor's package.json, so it doesn't have to be installed globally, which could break other projects.

Honza

Comment 6

2 years ago
mozreview-review
Comment on attachment 8905932 [details]
Bug 1395834 - Launchpad is broken;

https://reviewboard.mozilla.org/r/177728/#review182956

::: devtools/client/netmonitor/package.json:22
(Diff revision 1)
>      "react": "=15.3.2",
>      "react-dom": "=15.3.2",
>      "react-redux": "=5.0.3",
>      "redux": "^3.6.0",
> -    "reselect": "^2.5.4"
> +    "reselect": "^2.5.4",
> +    "webpack": "^3.5.6"

Is this webpack dependency necessary? I'm sure webpack 3 has been installed in devtools-launchpad already [1], and I don't see console [2] requires it as well. Perphaps we can just remove it.


BTW, webpack should be put in devDependencies. You can install it easily by `yarn add -D`.

[1] https://github.com/devtools-html/devtools-core/blob/master/packages/devtools-launchpad/package.json#L95
[2] http://searchfox.org/mozilla-central/rev/d29c832536839b60a9ee5b512eeb934274b879b0/devtools/client/webconsole/package.json
(In reply to Ricky Chien [:rickychien] from comment #6)
> Is this webpack dependency necessary? I'm sure webpack 3 has been installed
> in devtools-launchpad already [1], and I don't see console [2] requires it
> as well. Perphaps we can just remove it.
So, there is something I didn't probably understand well.

To make sure, if webpack 3 is installed locally in the Launchpad dir, all `yarn start` commands (from console, net, inspector, ect.) are using that no matter what is the version of globally installed webpack, correct?
If true, than yes we should remove it!

Honza
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(In reply to Ricky Chien [:rickychien] from comment #6)
> Is this webpack dependency necessary?
Removed

Honza
Related bug (webpack broken again): bug 1399390 

Honza
Reporter

Comment 12

2 years ago
mozreview-review
Comment on attachment 8905960 [details]
Bug 1395834 - Implement theme loader;

https://reviewboard.mozilla.org/r/177730/#review184426

Looking good, only needs to add/modify some comments

::: devtools/client/shared/webpack/theme-loader.js:5
(Diff revision 2)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +// Remove the "raw!" prefix used in some require which confuses webpack.

it can be "theme-loader" as well right ?

::: devtools/client/shared/webpack/theme-loader.js:19
(Diff revision 2)
> +  }
> +
> +  let request = remainingRequest.split("!");
> +  let rawUrl = request[request.length - 1];
> +  let content = fs.readFileSync(rawUrl, "utf8");
> +  content = content.replace("'", '"');

could we add a comment explining why we switch from somple to double quotes ?

::: devtools/shared/loader-plugin-raw.jsm:12
(Diff revision 2)
>  const { utils: Cu } = Components;
>  const { NetUtil } = Cu.import("resource://gre/modules/NetUtil.jsm", {});
>  
>  /**
>   * A function that can be used as part of a require hook for a
>   * loader.js Loader.  This function only handles webpack-style "raw!"

update the comment to say that it handle theme-loader prefix as well
Attachment #8905960 - Flags: review?(nchevobbe) → review+
Reporter

Comment 14

2 years ago
mozreview-review
Comment on attachment 8905932 [details]
Bug 1395834 - Launchpad is broken;

https://reviewboard.mozilla.org/r/177728/#review184470

One small typo, but everything is good

::: devtools/client/netmonitor/webpack.config.js:163
(Diff revision 2)
> +  babelExcludes: new RegExp(`^${basePath}(.(?!${baseName}))*$`)
> +});
>  
>  // Remove loaders from devtools-launchpad's webpack.config.js
>  // * For svg-inline loader:
> -//   Netmonitor uses file loader to bundle image assets instead of svg-inline loader
> +//   Webconsole uses file loader to bundle image assets instead of svg-inline-loader

s/WebConsole/Netmonitor
Attachment #8905932 - Flags: review?(nchevobbe) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #14)
> s/WebConsole/Netmonitor
Fixed

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #12)
> it can be "theme-loader" as well right ?
Ah, comment updated.

> ::: devtools/client/shared/webpack/theme-loader.js:19

> could we add a comment explining why we switch from somple to double quotes ?
Done

> ::: devtools/shared/loader-plugin-raw.jsm:12
> update the comment to say that it handle theme-loader prefix as well
Done

Thanks for the review Nicolas!

Honza
https://hg.mozilla.org/mozilla-central/rev/14a79a4e94a2
https://hg.mozilla.org/mozilla-central/rev/c71a1ac3a0f2
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57

Updated

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