1497839 - Use JSX for App component

Status: NEW

(DevTools :: Netmonitor, enhancement, P3)

Description

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

Since NodeJS can be used for Firefox build process (see bug 1464123), let's try to use JSX for the top level `App` component in the Network monitor panel.

The render method:
https://searchfox.org/mozilla-central/rev/80ac71c1c54af788b32e851192dfd2de2ec18e18/devtools/client/netmonitor/src/components/App.js#60

This bug should help to figure out how to do it:
Bug 1461714 (node-debugger) - Build debugger from sources in mozilla-central / allow running node scripts to process JS files

and 

Bug 1496126 - Build the whole debugger from sources in mozilla-central

Honza

Comment 1

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

@Alex: any instructions about how to do this?

Honza

Comment 2

[Alexandre Poirot [:ochameau]]

I would wait for the debugger to dogfood that first. See if that scale. For now it only builds one module and I'm looking forward how it will work when building the whole debugger sources (bug 1496126).

Otherwise I don't have any idea yet if we should reuse debugger build instructions as-is, fork them, move them to devtools/shared of something, ...

But here is how it works.

Everything starts from these moz.build instructions:
https://searchfox.org/mozilla-central/rev/80ac71c1c54af788b32e851192dfd2de2ec18e18/devtools/client/debugger/new/src/moz.build#16-18
DebuggerModules(
    'main.js',
)

It means that main.js is a special file that has to be processed by the node script.
DebuggerModules is being implemented here:
https://searchfox.org/mozilla-central/rev/80ac71c1c54af788b32e851192dfd2de2ec18e18/devtools/client/debugger/new/node-templates.mozbuild#7-35
But the main take away from this function is this line:
    bundle.inputs = ['/devtools/client/debugger/new/build/copy-module.js']
It says which nodejs script should be executed, i.e:
  https://searchfox.org/mozilla-central/source/devtools/client/debugger/new/build/copy-module.js

This is where I think, this is very specific to the debugger.
It contains code to convert the es-modules to devtools-loader-compatible modules as well as changing some require paths, and convert JSX to JS.

I think you should be able to prototype something out of these explanations.
I'm open to suggestions on how to share/not share these builds scripts.

Comment 3

[Alexandre Poirot [:ochameau]]

Oh one last thing, there is a whilelist of accepted nodejs script that moz.build can execute.
It is being specified here and require build peers review:
https://searchfox.org/mozilla-central/rev/80ac71c1c54af788b32e851192dfd2de2ec18e18/python/mozbuild/mozbuild/action/node.py#12-14
SCRIPT_ALLOWLIST = [
        buildconfig.topsrcdir + "/devtools/client/debugger/new/build/copy-module.js"
    ]

Comment 4

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

Attachment: net-build.patch

Thanks for the info Alex!

(In reply to Alexandre Poirot [:ochameau] from comment #2)
> I would wait for the debugger to dogfood that first. See if that scale. For
> now it only builds one module and I'm looking forward how it will work when
> building the whole debugger sources (bug 1496126).
Yes, David will be looking at the Debugger piece and I wanted to experiment a bit in context of another panel. This way we can have better picture about the top level requirements and see how to reuse the code among tools/panels.

For now I just cloned what you did for Debugger and I was able to transpile couple of existing Network monitor components that are using JSX.

Specifically:
netmonitor/src/components/StatusCode.js
netmonitor/src/components/RequestListColumnStatus.js'

But, when I try to transpile also:
netmonitor/src/components/RequestListContent.js

I am getting an error:
TypeError: undefined is not a non-null object builtin-modules.js:85 

It's because of:

loader.lazyGetter(this, "setImageTooltip", function() {
  return require("devtools/client/shared/widgets/tooltip/ImageTooltipHelper")
    .setImageTooltip;
});

defineLazyGetter function expects the module (this) as the first argument and it's null from some reason. Does that ring any bells?

You can see the error in Browser Console when applying this patch and opening the Network panel + reload.

Honza

Comment 5

[Alexandre Poirot [:ochameau]]

You may try to open resource://devtools/client/netmonitor/src/components/RequestListContent.js to see what it generated
Or look for RequestListContent.js in your objdir.

I imagine you are hitting some extra feature of the debugger build script that you don't want:
  https://searchfox.org/mozilla-central/source/devtools/client/debugger/new/build/copy-module.js
It process JSX, but also tries to compile es-modules to devtools modules while hacking some require paths.
It may do some other things I'm not even aware of...

May be you are having conflicts because one of the enabled plugins:
  https://searchfox.org/mozilla-central/source/devtools/client/debugger/new/build/copy-module.js#202-207

Comment 6

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

Just for the record, some tips from Alex on Slack:

• How to verify it works? Open the equivalent resource:// url in firefox to see if the file exists and is transpiled to the right content. Or read the file from the objdir. Transpiled files are here: objdir/devtools/client/...
• How to debug it? Run `./mach build devtools/client/...` You should be able to see exception if the script throws as well as console.logs.
• Can we log in it? Yes.

You may first try to run it manually via `node my-script.js my-file.js`.
The node script is expected to receive a list of argument which are the file to transpile. It should take these file and override them with the transpile content.

The second expectation is that it should prints special lines (via console.log), mentioning all dependencies. It includes the script itself, any build dependency like babel module and the files it transpiles.

I think the debugger build script is easy enough to understand that part: https://searchfox.org/mozilla-central/source/devtools/client/debugger/new/build/copy-module.js#215-230

Last expectation, the node script should only require() standard node modules *or* local JS files that are in-tree. No npm/yarn/whatever.

Dan is working on introducing npm support in m-c, but it will be for later. No ETA yet about that.

Comment 7

[David Walsh :davidwalsh]

Attachment: Bug 1497839 - Provide method for NetMonitor to use JSX

Comment 8

[Alexandre Poirot [:ochameau]]

Sorry David if yesterday meeting felt a bit harsh. Communication around the node build step wasn't clear.
I wasn't aware of this Outreachy JSX project at all, nor that we were actively trying to expand the node build step to other tools.

Your patch looks pretty good, I'll submit some suggestion on phabricator but I'm really happy to see you being able to hack this totally undocument and very cryptic moz.build bits and apply it to netmonitor :)

Unfortunately, the node build step doesn't scale because of:

• performance reasons,
• generated code downsides.

Performance issue

I did my best in the original bug introducing the node build step to be the most efficient.
But, we are still running node and the build script once per folder. We were about to do it once per file, so it is already better than worse. Executing node process and evaluating babel is taking a significant time and introduces a significant overhead if you do it many times. And the issue is that this build step is mandatory for anyone building firefox, not only DevTools contributors. So when you are slowing this down, you are making a significant part of the company slower.

For the debugger, we moved forward with such performance bottleneck as we were looking forward being able to build debugger from m-c and unlock all the initiatives to move from Github to mozilla-central (smoother debugger releases, being able to contribute from m-c, ...).
Here for netmonitor, the situation is different as that's only for JSX.
This is less critical than being able to build the debugger from m-c.

Generated code

This issue did not applied to the debugger as the code was already generated anyway.
But here, we start introducing differences between sources and runtime in a new codebase which wasn't having this issue.
We are still missing tooling like source-map for mozilla-central codebase in order to have original lines/columns.
For example, stack traces reported by users on bugzilla would be harder to interpret. Same thing for stack traces on test failures, ...
This is a pretty challenging thing to address as we have to:

• ensure that we do generate source-maps,
• ensure that all the places where we display stack traces are taking source map into account. It isn't clear if test failures are logging test failures with original locations.

This issue of generated code isn't so much about the patch you wrote, but more about the goal behind it.

Overall I think we ought to improve this nodejs build trick if we want to spread it over devtools:

• improve the build speed,
• demonstrate that we can have source-maps for the debugger and ensure original location are displayed in important locations,
• review what I just copied from debugger repo (babel.js, copy-module.js). Bastien comment is a good example of that. We should probably try to land the unminified sources. Or at very least document how we generated this file in order to be able to reproduce it and verify we have the same output.
• improve the log output of ./mach build. It is currently quite cryptic today with all these "node.stub". But this is more a nice to have.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7f51bc3757d9
Provide method for NetMonitor to use JSX r=jlast

Comment 10

[Cristian Brindusan [:cbrindusan]]

Backed out changeset 7f51bc3757d9 (Bug 1497839) for ESlint failure at build-debugger.js.

Backout: https://hg.mozilla.org/integration/autoland/rev/09dd6c2c111962afd8ef36a50184b4b3fa740c7e

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=7f51bc3757d9d196730d75d719199b29c21fbe0b&selectedJob=232064836

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=232064836&repo=autoland&lineNumber=279

Comment 11

[Jason Laster [:jlast]]

Attachment: Revert "Backed out changeset 7f51bc3757d9 (bug 1497839) for ESlint failure at build-debugger.js. CLOSED TREE"

This reverts commit 2b1fea9435191242f8aadc04da3ed1b0e0d99b02.

Bug 1497839 - fix eslint issues for babel build. r=jlast

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c36dc28ad5d0
Revert "Backed out changeset 7f51bc3757d9 for ESlint failure at build-debugger.js. CLOSED TREE"

Comment 13

[Alexandre Poirot [:ochameau]]

(it would be great if you could keep the history of node-templates.mozbuild file [by using hg mv or something])