1359448 - Lock down devtools-core package

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect, P1)

Description

[Ricky Chien [:rickychien] (inactive)]

There are a few update and bug fixed in devtools-core recently (export cause, fromCache, fromServiceWorker [1].


[1] https://github.com/devtools-html/devtools-core/pull/344

Comment 1

[Ricky Chien [:rickychien] (inactive)]

qe-verify-

Comment 2

[Ricky Chien [:rickychien] (inactive)]

Latest change of devtools-connection module in https://github.com/devtools-html/devtools-core/pull/344 hasn't published to NPM yet at the time. Let's wait for devtools-connection version bump and review again.

Comment 3

[Ricky Chien [:rickychien] (inactive)]

Attachment: Bug 1359448 - Lock down devtools-core package

Review commit: https://reviewboard.mozilla.org/r/134564/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/134564/

Comment 4

[Ricky Chien [:rickychien] (inactive)]

Filed bump package request at https://github.com/devtools-html/devtools-core/issues/349.

Bumped devtools-connection https://github.com/devtools-html/devtools-core/commit/054f8992e47234425d42d9fc014e57bcc7e3c1df

Bumped launchpad https://github.com/devtools-html/devtools-core/commit/8a12fbfd3996051dd375f6badbc84d355afe2beb

Thanks Jason's help!

Comment 5

[[e-mail]]

Comment on attachment 8862694 [details]
Bug 1359448 - Lock down devtools-core package

https://reviewboard.mozilla.org/r/134564/#review137556

r+ after nit fixed

::: devtools/client/netmonitor/index.js:16
(Diff revision 1)
>   */
>  const React = require("react");
>  const ReactDOM = require("react-dom");
>  const { bindActionCreators } = require("redux");
>  const { bootstrap, renderRoot } = require("devtools-launchpad");
> -const { EventEmitter } = require("devtools-modules");
> +const EventEmitter = require("devtools-connection/src/utils/event-emitter");

do we need to include `devtools-connection` module if we use it?

::: devtools/client/netmonitor/package.json:10
(Diff revision 1)
>      "node": ">=6.9.0"
>    },
>    "description": "Network monitor in developer tools",
>    "dependencies": {
>      "codemirror": "^5.24.2",
>      "devtools-config": "=0.0.12",

add "devtools-connection": "
0.04",

Comment 6

[[e-mail]]

Comment on attachment 8862694 [details]
Bug 1359448 - Lock down devtools-core package

https://reviewboard.mozilla.org/r/134564/#review137556

> add "devtools-connection": "
> 0.04",

And we should update the README.md accordingly when add a new dependency

Comment 7

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8862694 [details]
Bug 1359448 - Lock down devtools-core package

https://reviewboard.mozilla.org/r/134564/#review137556

> do we need to include `devtools-connection` module if we use it?

Good catch! 

After investigating, I think we are no longer to depend on devtools-connection since it's part of nested dependency of devtools-launchpad.

EventEmitter can also be accessed from devtools-module, as a result, we can get rid of it.

Thanks for spotting the issue!

> And we should update the README.md accordingly when add a new dependency

Likewise above. The netmonitor is no longer necessarily to depend on devtools-connection anymore, READMD.md update will not be needed.

Comment 8

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8862694 [details]
Bug 1359448 - Lock down devtools-core package

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/134564/diff/1-2/

Comment 9

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8862694 [details]
Bug 1359448 - Lock down devtools-core package

https://reviewboard.mozilla.org/r/134564/#review137568

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2e7660e83f7d
Lock down devtools-core package r=gasolin

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2e7660e83f7d