Closed Bug 1395834 Opened 8 years ago Closed 8 years ago

Launchpad is broken

Categories

(DevTools :: Netmonitor, defect, P1)

Product:
DevTools
Component:
Netmonitor
Type:
defect

Tracking

(firefox57 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: nchevobbe, Assigned: Honza)

References

Details

Attachments

(2 files)

Bug 1395834 - Launchpad is broken;
59 bytes, text/x-review-board-request
nchevobbe
: review+
Details
Bug 1395834 - Implement theme loader;
59 bytes, text/x-review-board-request
nchevobbe
: review+
Details
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
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
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 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
(In reply to Ricky Chien [:rickychien] from comment #6) > Is this webpack dependency necessary? Removed Honza
Related bug (webpack broken again): bug 1399390 Honza
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+
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+
(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
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: