Closed Bug 1350217 Opened 7 years ago Closed 7 years ago

Introduce webpack.config to run netmonitor on browser tab

Categories

(DevTools :: Netmonitor, enhancement, P1)

Product:
DevTools
Component:
Netmonitor
Type:
enhancement

Tracking

(firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Iteration:
55.2 - Apr 3
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: gasolin, Assigned: rickychien)

References

Details

(Whiteboard: [netmonitor])

Attachments

(2 files)

Bug 1350217 - Introduce webpack.config.js
59 bytes, text/x-review-board-request
Honza
: review+
Details
Bug 1350217 - Remove redundant shared module dependencies
59 bytes, text/x-review-board-request
Honza
: review+
Details 
We plan to run netmonitor on both toolbar and browser tab

This bug would introduce files needed to run on browser tab

* devtools/client/netmonitor (Required for running on the launchpad)
- package.json
- webpack.config.js
- index.js or main.js
src/


* webpack.config.js should support file-loader
* main.js to bootstrap netmonitor.html
* Provide instructions (README.md) about how to run Netmonitor.html on browser tab
Depends on: 1350215
Priority: -- → P1
Whiteboard: [netmonitor]
Blocks: 1350223
Flags: qe-verify?
Priority: P1 → P2
Assignee: nobody → rchien
Status: NEW → ASSIGNED
Priority: P2 → P1
Iteration: --- → 55.2 - Apr 3
Some modules are ported to devtools-modules and devtools-sham-modules, check poc branch to see whats changed in require
https://github.com/gasolin/devtools-core/commits/netmonitor.html/packages/netmonitor
Flags: qe-verify? → qe-verify-
Comment on attachment 8852315 [details]
Bug 1350217 - Introduce webpack.config.js

https://reviewboard.mozilla.org/r/124564/#review127074

::: devtools/client/netmonitor/README.md:2
(Diff revision 2)
> +# netmonitor
> +

Add short description `Network monitor for developers.`

::: devtools/client/netmonitor/README.md:7
(Diff revision 2)
> +
> +## Prerequisite
> +
> +* [node] >= 6.9.x
> +* [npm] >= 3.x
> +

`yarn` is also required before debugging the netmonitor.html

```
$ npm install -g yarn
```

::: devtools/client/netmonitor/README.md:21
(Diff revision 2)
> +# Create a dev server instance for hosting netmonitor on browser
> +yarn start
> +
> +# Run firefox
> +firefox http://localhost:8000 --start-debugger-server 6080
> +```

`Then open localhost:8000 in any browser to see all tabs in Firefox.`

::: devtools/client/netmonitor/configs/development.json:7
(Diff revision 2)
> +  "title": "Netmonitor",
> +  "environment": "development",
> +  "baseWorkerURL": "http://localhost:8000/public/build/",
> +  "host": "",
> +  "theme": "light",
> +  "logging": {

we can add `"dir": "ltr",` to ease the RTL test

::: devtools/client/netmonitor/index.js:24
(Diff revision 2)
> +// require("./src/assets/styles/netmonitor.css");
> +
> +EventEmitter.decorate(window);
> +
> +const App = require("./src/components/app");
> +const store = configureStore();

const store = window.gStore = configureStore();

::: devtools/client/netmonitor/src/components/request-list-content.js
(Diff revision 2)
>   * 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/. */
>  
>  "use strict";
>  
> -const { KeyCodes } = require("devtools/client/shared/keycodes");

dont need to change this file since we can import from devtools-sham-modules and do the alias

::: devtools/client/netmonitor/src/components/request-list-header.js
(Diff revision 2)
>    createClass,
>    PropTypes,
>    DOM,
>  } = require("devtools/client/shared/vendor/react");
>  const { connect } = require("devtools/client/shared/vendor/react-redux");
> -const { setNamedTimeout } = require("devtools/client/shared/widgets/view-helpers");

don't need to change this file since we can import from devtools-sham-modules and do the alias
Comment on attachment 8852315 [details]
Bug 1350217 - Introduce webpack.config.js

https://reviewboard.mozilla.org/r/124564/#review127102

::: devtools/client/netmonitor/package.json:30
(Diff revision 2)
> -    "jsdom-global": "^2.0.0",
> -    "mocha": "^2.5.3",
> -    "require-hacker": "^2.1.4"
>    },
>    "scripts": {
> -    "postinstall": "cd ../ && npm install && cd netmonitor",
> +    "start": "node bin/dev-server"

Though its not a high priority item, remove those devdependencies and the `postinstall` and `test` scripts will broke component tests with emzyme.
Comment on attachment 8852315 [details]
Bug 1350217 - Introduce webpack.config.js

https://reviewboard.mozilla.org/r/124564/#review127074

> dont need to change this file since we can import from devtools-sham-modules and do the alias

Yep, I'll move this part to a separate patch. What I'm concerned about this is that we can solve and remove this useless module dependency from codebase by adopting web standard APIs. Like using evt.key instead of using constant KeyCodes library.

Keycode is suggested to be deprecated https://developer.mozilla.org/zh-TW/docs/Web/API/KeyboardEvent/keyCode
Key is the recommended option https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/key

We still can import the sham-modules into devtools-core but we also can improve our code in the meantime.

> don't need to change this file since we can import from devtools-sham-modules and do the alias

Like wise above.
Comment on attachment 8852315 [details]
Bug 1350217 - Introduce webpack.config.js

https://reviewboard.mozilla.org/r/124564/#review127106

::: devtools/client/netmonitor/index.js:25
(Diff revision 2)
> +
> +EventEmitter.decorate(window);
> +
> +const App = require("./src/components/app");
> +const store = configureStore();
> +const actions = bindActionCreators(require("./src/actions"), store.dispatch);

We didn't use the bindActionCreators in our codebase, it might worth to take a look about how JSONViewer use bootstrap to load react components implicitly without depends on launchpad's magical bootstrap
https://github.com/devtools-html/devtools-core/pull/202/files

You can also refer how I do so in another netmonitor poc
https://github.com/gasolin/devtools-core/blob/netmonitor.html/packages/netmonitor/src/main.js
(In reply to Fred Lin [:gasolin] from comment #5)
> Though its not a high priority item, remove those devdependencies and the
> `postinstall` and `test` scripts will broke component tests with emzyme.

Hmm, good question. However, our enzyme testing hasn't enabled yet and at the beginning we copy these settings from console. Now I saw there are different settings in debugger.html https://github.com/devtools-html/debugger.html/blob/master/package.json, so I'd prefer to remove this temporary since it's probably outdated, but enzyme testing have to re-enable in someday and we can learn from how debugger manage their setting and rewrite a new config for it.
(In reply to Fred Lin [:gasolin] from comment #7)
> We didn't use the bindActionCreators in our codebase, it might worth to take
> a look about how JSONViewer use bootstrap to load react components
> implicitly without depends on launchpad's magical bootstrap
> https://github.com/devtools-html/devtools-core/pull/202/files

Yep, I've removed bindActionCreators in patch.

Note that we're not going to make netmonitor runs on browser in this bug, the connection between NetMonitorController and launchpad expect not to work in this stage. Let's wait for the porting of devtools-core which should solve webpack errors, and then we can move forward to see how to connect launchpad to controller within index.js.

Thanks!
Comment on attachment 8852315 [details]
Bug 1350217 - Introduce webpack.config.js

https://reviewboard.mozilla.org/r/124564/#review127126

::: devtools/client/netmonitor/webpack.config.js:58
(Diff revision 3)
> +      "devtools/client/shared/vendor/redux": "redux",
> +      "devtools/client/shared/vendor/reselect": "reselect",
> +      "devtools/client/shared/vendor/jszip": "jszip",
> +      "devtools/client/shared/redux/middleware/thunk": "redux-thunk",
> +      "devtools/client/locales": path.join(__dirname, "../locales/en-US"),
> +      "Services": path.join(__dirname, "../shared/shim/Services.js"),

"Services": "devtools-modules",
Comment on attachment 8852315 [details]
Bug 1350217 - Introduce webpack.config.js

https://reviewboard.mozilla.org/r/124564/#review127508

::: devtools/client/netmonitor/.gitignore:1
(Diff revision 7)
> +build/

Why do we need this file? (we are not on github)
Comment on attachment 8852351 [details]
Bug 1350217 - Remove redundant shared module dependencies

https://reviewboard.mozilla.org/r/124608/#review127512
Attachment #8852351 - Flags: review?(odvarko) → review+
Comment on attachment 8852315 [details]
Bug 1350217 - Introduce webpack.config.js

https://reviewboard.mozilla.org/r/124564/#review127586

R+, but .gitignore file should be removed (the development still happens in m-c and .hgignore has what we need including line for node_modules)


Thanks Ricky!
Honza
Attachment #8852315 - Flags: review?(odvarko) → review+
Thank you Honza!

Merged webpack.config.js from https://bugzilla.mozilla.org/show_bug.cgi?id=1350222#c2 and remove .gitignore as comment 24 mentioned.

Let's wait for try green.
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5d6386b7378c
Remove redundant shared module dependencies r=Honza
https://hg.mozilla.org/integration/autoland/rev/6b83bff50da9
Introduce webpack.config.js r=Honza
https://hg.mozilla.org/mozilla-central/rev/5d6386b7378c
https://hg.mozilla.org/mozilla-central/rev/6b83bff50da9
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Depends on: 1352288
Blocks: 1352699
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: