1410782 - Fix props.connector is undefined when running launchpad

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect, P1)

Description

[Ricky Chien [:rickychien] (inactive)]

I'm seeing TypeError: props.connector is undefined in console when running netmonitor on launchpad. This is a regression after refactoring connector from a singleton into instance.

Now we passed an extra prop into renderRoot function [1] and modified renderRoot in devtools-launchpad [2]. However, launchpad workflow stop working since we forgot updating devtools-launchpad version.

[1] https://searchfox.org/mozilla-central/rev/8a6a6bef7c54425970aa4fb039cc6463a19c0b7f/devtools/client/netmonitor/index.js#85

See also https://github.com/devtools-html/devtools-core/pull/727/files

Comment 1

[Ricky Chien [:rickychien] (inactive)]

Attachment: Bug 1410782 - Fix props.connector is undefined when running launchpad

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

Comment 2

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Nicolas: I am seeing this error when upgrading to the latest Launchpad. Any tips how to solve?
Is it because Flow is not supported properly?

Honza

ERROR in ./node_modules/devtools-launchpad/assets/Svg.js
Module build failed: SyntaxError: C:/src/mozilla.org/mozilla-central/devtools/client/netmonitor/no
de_modules/devtools-launchpad/assets/Svg.js: Unexpected token, expected ; (9:5)

   7 | };
   8 |
>  9 | type SvgType = {
     |      ^
  10 |   name: string,
  11 |   className?: string,
  12 |   onClick?: () => void,

 @ ./node_modules/devtools-launchpad/src/components/Sidebar.js 9:12-42
 @ ./node_modules/devtools-launchpad/src/components/LandingPage.js
 @ ./node_modules/devtools-launchpad/src/components/LaunchpadApp.js
 @ ./node_modules/devtools-launchpad/src/index.js
 @ ./index.js
 @ multi ./index.js

Comment 3

[Julien Wajsberg [:julienw]]

Looks like we can't use the source files directly, we need to publish a "dist" directory and use it.

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

It does look like it's a flow issue, but the config in launchpad should not produce this error https://github.com/devtools-html/devtools-core/blob/master/packages/devtools-launchpad/package.json#L40 ?
Maybe the babel plugin needs to be defined in the netmonitor babel config as well ?

Comment 5

[Ricky Chien [:rickychien] (inactive)]

I suspect that could be we skipped babel in netmonitor webpack.config.js

https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/webpack.config.js#137

Comment 6

[Ricky Chien [:rickychien] (inactive)]

Removing babelExcludes as comment 5 and adding babel-plugin-transform-flow-strip-types plugin doesn't fix this issue.

I'm in a favor of pushing a "dist" directory to avoid any parsing issue in the future. On the other hand, it doesn't make sense to install a babel-plugin-transform-flow-strip-types package in netmonitor since all netmonitor/src/* doesn't write with flow.

Comment 7

[Ricky Chien [:rickychien] (inactive)]

My observation is that the root cause comes from devtools-launchpad@0.0.104 [1]. 0.0.104 start using postcss instead of postcss-js and added lots of stuff related to webpack.config. Using 0.0.103 works for me but some style, UI broken.

list all I found after switching over different versions

devtools-launchpad@0.0.99: Style / UI broken (devtools-launchpad's theme API changes)

devtools-launchpad@0.0.101: "props.connector is undefined" error get fixed

devtools-launchpad@0.0.104: "No PostCSS Config found" error appeared

devtools-launchpad@0.0.106: Flow syntax error like "SyntaxError: Unexpected token, expected ;" appeared

[1] https://github.com/devtools-html/devtools-core/commit/e4f43c188f5066ba01a082dbb86267561b3563b1

Jason, there are so many breaking changes in devtools-launchpad. It would be hard for us to keep up with the latest version. Could you help us point out how to fix these issues?

Comment 8

[Ricky Chien [:rickychien] (inactive)]

At least we can stick with devtools-launchpad@0.0.103 without props.connector issue but try to fix styling issue for 0.0.99.

Comment 9

[Ricky Chien [:rickychien] (inactive)]

devtools-launchpad@0.0.98 UI is fine but it is broken in 0.0.99

Here are commit changes during 0.0.98 ~ 0.0.99 (55 commits & 102 files changed)
 
https://github.com/devtools-html/devtools-core/compare/9b5a76...267ad91

Comment 10

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8920967 [details]
Bug 1410782 - Fix props.connector is undefined when running launchpad

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

Comment 11

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1410782 - Fix props.connector is undefined when running launchpad

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

Comment 12

[Ricky Chien [:rickychien] (inactive)]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #11)
> Created attachment 8921452 [details]
> Bug 1410782 - Fix props.connector is undefined when running launchpad

Thanks for your help \o/ I can see lots of issues are fixed but some images cannot display correctly. Let's me check and I'll submit patch soon.

Comment 13

[[e-mail]]

Comment on attachment 8921452 [details]
Bug 1410782 - Fix props.connector is undefined when running launchpad

https://reviewboard.mozilla.org/r/192496/#review197926

It works \o/

::: devtools/client/netmonitor/.babelrc:2
(Diff revision 1)
> +{
> +  "presets": ["react", "es2017"],

we can use https://github.com/babel/babel-preset-env instead of specify es2015~es2017, or we can list es2015, es2016, es2017 incase we miss the exponentiation (**) in es2016 someday https://babeljs.io/docs/plugins/preset-es2016/

Comment 14

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8920967 [details]
Bug 1410782 - Fix props.connector is undefined when running launchpad

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

Comment 15

[Ricky Chien [:rickychien] (inactive)]

(In reply to Fred Lin [:gasolin] from comment #13)
> we can use https://github.com/babel/babel-preset-env instead of specify
> es2015~es2017, or we can list es2015, es2016, es2017 incase we miss the
> exponentiation (**) in es2016 someday
> https://babeljs.io/docs/plugins/preset-es2016/

I tried babel-preset-env but unluckily it doesn't work and threw generator runtime issue. Seems like we need to add extra setting in .babelrc to make it work. However, I'd suggest to stick to es2017 since our target browsers are Firefox & Chrome, I believe it's okay to only introduce es2017 at the moment (babel-preset-es2017 could be removed soon once browsers are getting stable in the near future).


Note that current patch still has CSS variable issue. Some icons are missing if the url(chrome://...) is defined as a CSS variable. But I don't know why it works in toolbox but broken in launchpad.

Comment 16

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8921452 [details]
Bug 1410782 - Fix props.connector is undefined when running launchpad

Obsolete this patch in case confusion.

Comment 17

[Julien Wajsberg [:julienw]]

(In reply to Ricky Chien [:rickychien] from comment #15)
> (In reply to Fred Lin [:gasolin] from comment #13)
> > we can use https://github.com/babel/babel-preset-env instead of specify
> > es2015~es2017, or we can list es2015, es2016, es2017 incase we miss the
> > exponentiation (**) in es2016 someday
> > https://babeljs.io/docs/plugins/preset-es2016/
> 
> I tried babel-preset-env but unluckily it doesn't work and threw generator
> runtime issue.

That's because by default it transpiles for all browsers, including the bad ones :) So it makes babel configured to implement generators with a generator runtime you need to include in the project.

Good news! Firefox doesn't need this as it implements generators natively! So just configure "env" by specifying latest version of Firefox (and/or Chrome if you want too). You can use the following "browsers" string to achieve this:
  "last 1 firefox version, last 1 chrome version"

Note you'll need to keep the lists uptodate by properly updating versions of babel-preset-env (who themselves update their compat-table versions).

>  Seems like we need to add extra setting in .babelrc to make
> it work. However, I'd suggest to stick to es2017 since our target browsers
> are Firefox & Chrome, I believe it's okay to only introduce es2017 at the
> moment (babel-preset-es2017 could be removed soon once browsers are getting
> stable in the near future).

"env" support the same as "latest" which supports "es2015, es2016, es2017". I think it's fine, as they are the specified stuff. As long as we specify also the browser strings we shouldn't do things that are useless.

I strongly suggest to just use the "env" preset properly configured.

Comment 18

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8920967 [details]
Bug 1410782 - Fix props.connector is undefined when running launchpad

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/191940/diff/3-4/

Comment 19

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8920967 [details]
Bug 1410782 - Fix props.connector is undefined when running launchpad

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/191940/diff/4-5/

Comment 20

[Ricky Chien [:rickychien] (inactive)]

The most of the import part of this patch is to disable PostCSS loader

// Disable PostCSS loader
config.module.rules.forEach(rule => {
  if (Array.isArray(rule.use)) {
    rule.use.some((use, idx) => {
      if (use.loader === "postcss-loader") {
        rule.use = rule.use.slice(0, idx);
        return true;
      }
      return false;
    });
  }
});

* It turns out that all of those babel preset plugins, babelrc, postcss.config.js are unnecessary.
* Using 0.0.103 is able to get rid of missing icon issues. (only issue I can see now is a weird black background in filter input)

Comment 21

[[e-mail]]

Comment on attachment 8920967 [details]
Bug 1410782 - Fix props.connector is undefined when running launchpad

https://reviewboard.mozilla.org/r/191940/#review198024

::: devtools/client/netmonitor/package.json:18
(Diff revision 5)
> -    "devtools-modules": "=0.0.31",
> +    "devtools-modules": "=0.0.32",
>      "devtools-source-editor": "=0.0.3",
>      "immutable": "^3.8.1",
>      "jszip": "^3.1.3",
>      "react": "=15.3.2",
>      "react-dom": "=15.3.2",

Bug 1399493 is landed, we can use this chance to update react version to 15.6.1 as well, or we can do it in a separate bug

Comment 22

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8920967 [details]
Bug 1410782 - Fix props.connector is undefined when running launchpad

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/191940/diff/5-6/

Comment 23

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8920967 [details]
Bug 1410782 - Fix props.connector is undefined when running launchpad

https://reviewboard.mozilla.org/r/191940/#review198024

> Bug 1399493 is landed, we can use this chance to update react version to 15.6.1 as well, or we can do it in a separate bug

Fixed.

Comment 24

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8920967 [details]
Bug 1410782 - Fix props.connector is undefined when running launchpad

https://reviewboard.mozilla.org/r/191940/#review198048

::: devtools/client/netmonitor/webpack.config.js:143
(Diff revision 6)
> +// For PostCSS loader:
> +// Disable PostCSS loader
> +config.module.rules.forEach(rule => {
> +  if (Array.isArray(rule.use)) {
> +    rule.use.some((use, idx) => {
> +      if (use.loader === "postcss-loader") {
> +        rule.use = rule.use.slice(0, idx);
> +        return true;
> +      }
> +      return false;
> +    });
> +  }
> +});

I wish it would be fixed by the launchpad itself, because it is very brittle.
Can you file an issue against launchpad to allow not having the postCSS loader (or an exclude array with all the loader we don't want, so we can handle svg-inline-loader as well), and add the reference in the comment with a TODO ?

Comment 25

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8920967 [details]
Bug 1410782 - Fix props.connector is undefined when running launchpad

https://reviewboard.mozilla.org/r/191940/#review198058

I am seeing this error:

yarn start v0.22.0
$ node bin/dev-server
Listening for WS on localhost:8116, all traffic is proxied to localhost:6080
Protocol messages can be logged by enabling logging.firefoxProtocol in your loca
l.json config
Hot Reloading - https://github.com/devtools-html/debugger.html/blob/master/docs/
local-development.md#hot-reloading
Development Server Listening at http://localhost:8000

webpack: Compiled successfully.
_http_outgoing.js:492
    throw new TypeError('The header content contains invalid characters');
    ^

TypeError: The header content contains invalid characters
    at validateHeader (_http_outgoing.js:492:11)
    at ServerResponse.setHeader (_http_outgoing.js:496:3)
    at C:\src\mozilla.org\mozilla-central\devtools\client\netmonitor\node_module
s\express-static\index.js:48:11
    at FSReqWrap.oncomplete (fs.js:153:5)
error Command failed with exit code 1.



Honza

Comment 26

[Ricky Chien [:rickychien] (inactive)]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #24)
> I wish it would be fixed by the launchpad itself, because it is very brittle.
> Can you file an issue against launchpad to allow not having the postCSS
> loader (or an exclude array with all the loader we don't want, so we can
> handle svg-inline-loader as well), and add the reference in the comment with
> a TODO ?

Yep, I agree that this way is more elegant and flexible. But note that Netmonitor is unable to stick with latest devtools-launchpad version (we are using 0.0.103 in this patch to prevent icon missing issue).

Comment 27

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Re: comment #25, I am only seeing the problem on Win10 (OSX works fine)

Honza

Comment 28

[Ricky Chien [:rickychien] (inactive)]

(In reply to Jan Honza Odvarko [:Honza] from comment #25)
> webpack: Compiled successfully.
> _http_outgoing.js:492
>     throw new TypeError('The header content contains invalid characters');
>     ^
> 
> TypeError: The header content contains invalid characters
>     at validateHeader (_http_outgoing.js:492:11)
>     at ServerResponse.setHeader (_http_outgoing.js:496:3)
>     at
> C:\src\mozilla.org\mozilla-central\devtools\client\netmonitor\node_module
> s\express-static\index.js:48:11
>     at FSReqWrap.oncomplete (fs.js:153:5)
> error Command failed with exit code 1.

According to your error msg, I suspect the root cause is due to express-static@1.2.4. AFAIK, express-static is a new package we added in devtools-launchpad@0.0.101 [1] and there is no newer version of express-static which could fixed the issue on Windows.

Sadly, our `props` fix for devtools-launchpad is done during 0.0.101 cycle [2] and published in 0.0.102. As a result, we cannot go back to 0.0.101 for Windows compatibility.

Honza, I'd like to land this workaround without waiting for express-static fix for Windows. Upgrading to latest devtools-launchpad is also troublesome as well. Perhaps we can follow SemVer to publish devtools-core packages to mitigate this kind of issue easily in the future.

[1] https://github.com/devtools-html/devtools-core/blame/e1bd9fd491b41fd96dc0c0c3e652327b0b847d3a/packages/devtools-launchpad/package.json#L60
[2] https://github.com/devtools-html/devtools-core/commit/f2955c5e4c128d8e3383dbb5e883092bcf2763b7

Comment 29

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8920967 [details]
Bug 1410782 - Fix props.connector is undefined when running launchpad

https://reviewboard.mozilla.org/r/191940/#review198524

> Honza, I'd like to land this workaround without waiting for express-static fix for Windows. Upgrading to latest devtools-launchpad is also troublesome as well. Perhaps we can follow SemVer to publish devtools-core packages to mitigate this kind of issue easily in the future.

Agree, let's not blog on the win problem if it fixes (at least) OSX.
R+, but please make sure there is a follow up

Thanks for working on this Ricky!

Honza

Comment 30

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Jan Honza Odvarko [:Honza] from comment #29)
> Agree, let's not blog
* block :-)

Honza

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/df69a6cc339b
Fix props.connector is undefined when running launchpad r=Honza

Comment 32

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/df69a6cc339b