Closed
Bug 1395834
Opened 7 years ago
Closed 7 years ago
Launchpad is broken
Categories
(DevTools :: Netmonitor, defect, P1)
DevTools
Netmonitor
Tracking
(firefox57 fixed)
RESOLVED
FIXED
Firefox 57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: nchevobbe, Assigned: Honza)
References
Details
Attachments
(2 files)
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 | ||
Updated•7 years ago
|
Assignee: nobody → odvarko
Status: NEW → ASSIGNED
Priority: -- → P1
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•7 years ago
|
||
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) |
Assignee | ||
Comment 4•7 years ago
|
||
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
Assignee | ||
Comment 5•7 years ago
|
||
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•7 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
Assignee | ||
Comment 7•7 years ago
|
||
(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) |
Assignee | ||
Comment 10•7 years ago
|
||
(In reply to Ricky Chien [:rickychien] from comment #6) > Is this webpack dependency necessary? Removed Honza
Assignee | ||
Comment 11•7 years ago
|
||
Related bug (webpack broken again): bug 1399390 Honza
Reporter | ||
Comment 12•7 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 13•7 years ago
|
||
mozreview-review |
Comment on attachment 8905960 [details] Bug 1395834 - Implement theme loader; https://reviewboard.mozilla.org/r/177730/#review184436
Reporter | ||
Comment 14•7 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) |
Assignee | ||
Comment 17•7 years ago
|
||
(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
Comment 18•7 years ago
|
||
Pushed by jodvarko@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/14a79a4e94a2 Launchpad is broken; r=nchevobbe https://hg.mozilla.org/integration/autoland/rev/c71a1ac3a0f2 Implement theme loader; r=nchevobbe
Comment 19•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/14a79a4e94a2 https://hg.mozilla.org/mozilla-central/rev/c71a1ac3a0f2
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•