Update/add new version of devtools-frontend in user tests

[TimvdLippe]

This issue is extracted from #38551 (comment)

Our test of an old version of devtools-frontend is an odd one for us. It has so many transitional patterns that expose bugs in our JS-handling code that we'd lose with an updated code base. On the other hand, it's likely that fewer and fewer people use those patterns -- probably we should just update and allow support for the old way to bit-rot.

by @sandersn

The background is that the current version of devtools-frontend predates our migration to ES modules. For reference, the currently used version is 530099:

TypeScript/tests/cases/user/chrome-devtools-frontend/package.json

Line 12 in 87fd182

"chrome-devtools-frontend": "1.0.530099"

That version was published on 2018-01-18. Since then, we have migrated to ES modules. Our NPM package hasn't fully been updated yet with that (CC @paulirish), but I recommend depending on a specific commit of our GitHub clone instead: https://github.com/chromedevtools/devtools-frontend

Why add a new version?

The usage of ES modules makes DevTools "easier" to typecheck. E.g. we are writing more idiomatic code that TypeScript expects. As such, it is better at understanding code and finding potential issues. However, it also uncovers issues in particularly the JSDoc parsing. Examples include #38551, #38550 and #38553

By adding a new version of devtools-frontend, it hopefully prevents such declaration issues from emerging before a new version of TypeScript is released.

Why keep the old version?

As @sandersn commented, there is value in keeping the older 2018 version around. The benefit would be the performance/usage of TypeScript on non-idomatic global-scope-sharing application code. I will leave that up to the TypeScript team to decide whether that is worth keeping around (with the increased maintenance, but potentially better at spotting regressions)

[DanielRosenwasser]

To me the answer is just "why not both?" Freeze one in time, track the latest as well, remove the older one if and when we feel like it's a burden. I guess the only issue is losing a bit of parallelization.

[sandersn]

We discussed this amongst the team last week and we thought that there was value in 3 points in time:

1. 530099, which tests global-scope-sharing javascript compilation.
2. a point after the ES module migration but before the typescript migration, to test compilation of a large, TS-checked javascript codebase.
3. The latest commit, to make sure that the devtools-frontend codebase itself can upgrade to new versions of Typescript without running into problems.

@TimvdLippe I saw that you recently started the conversion to Typescript. Is there a good commit for to use for (2) that is just prior to that?

[TimvdLippe]

https://crbug.com/1011811 is our canonical bug. I did some digging and https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/1960456 appears to be our very first integration of TypeScript (typechecking front_end/common/Trie.js). However, the compilation errors weren't fixed in that CL. That appears to be done in https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/1962288

So I guess if you take the parent of cbab43ca0aa3dc5a07b8f98086e6cec092e88b01 you would be at point 2.

For an overview of CLs that typecheck a specific existing JavaScript file with TypeScript, https://docs.google.com/spreadsheets/d/1QebRQuI0oTwMb2A6VddiuYD7WeUWd0QUkUMf5eMFQUg/edit?usp=sharing is our canonical spreadsheet.

[TimvdLippe]

Oh and in terms of configuration options, it is a combination of https://chromium.googlesource.com/devtools/devtools-frontend/+/refs/heads/master/tsconfig.base.json and https://chromium.googlesource.com/devtools/devtools-frontend/+/refs/heads/master/third_party/typescript/ts_library.py#101

[sandersn]

Oh, right. Unless the newer commits build with a single tsc invocation we'll need to use the docker test suite to run a real build.

[TimvdLippe]

We are using Ninja, as we desugar GN actions to to an invocation to tsc, which is the ts_library file I linked above.

We have two options:

1. You eagerly include all JS/TS and assume build options. This means that we can prevent problems in the future when we get further along with our migration. However, that puts a larger burden on you and will clutter your error log.
2. A better option would be to run the build. I can provide you instructions to optimize the build, since you only care about the TypeScript actions. As a starter, the following build should be quite efficient: autoninja -C out/Default front_end. We are making some changes tomorrow that will make sure all of TSC runs as part of that command (WIP CL https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/2292283). Today, you probably need to run autoninja -C out/Default :devtools_frontend_resources, but this will run the full build for DevTools including non-TypeScript actions.

Depending on the performance of your machine, a full build is expected to take about 2 minutes. We are still looking into tsc performance, so hopefully we can prune it down a bit in the future.

[weswigham]

If you can write a short dockerfile describing the build, it shouldn't be hard to add. The challenges are usually passing enough build options to make the build mostly-silent-except-TS-errors, since the builds are pass/failed by diffing the build logs (so reduced noise is beneficial), and figuring out where/how to inject the built package drop.

[TimvdLippe]

I created the DockerFile in #39624. I locally verified that removing the typescript/typescript copy steps it builds. I am not 100% sure that it also would correctly fail the build, as (ironically) our build passes with TypeScript atm. If the build would fail, then the autoninja -C out/Default front_end step would start to shout.

Presumably, when the build would fail, the process would exit with code 1. If I am not mistaken, we could do autoninja -C out/Default front_end > /dev/null which would only show up errors. It's probably best if you try it out locally with a change made to TypeScript that could cause DevTools to not typecheck and verify that at least 1 step would start shouting?

[TimvdLippe]

@weswigham The DockerFile landed a while ago. Are there any other actions we need to perform or can we consider this issue closed?

[weswigham]

Yeah, we can consider this closed.