variations: Migrate DevTools ClientVariations parser to TypeScript [chromium/src : main]
Alexei Svitkine (Gerrit)
Alexei Svitkine voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Andrzej Fiedukowicz
- Demetrios Papadopoulos
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Demetrios Papadopoulos (Gerrit)
Demetrios Papadopoulos added 1 comment![]( "Open in Gerrit")
- Alexei Svitkine
- Andrzej Fiedukowicz
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Demetrios Papadopoulos (Gerrit)
Demetrios Papadopoulos added 5 comments![]( "Open in Gerrit")
(globalThis as any).parseClientVariations = parseClientVariations;
(globalThis as any).formatClientVariations = formatClientVariations;Are these global exports really needed? This file is already using JS modules and the `export` keyword. Can whoever uses these not `import` them directly from here?
(globalThis as any).parseClientVariations = parseClientVariations;
(globalThis as any).formatClientVariations = formatClientVariations;We are actively trying to ban 'any' across the codebase, see https://issues.chromium.org/issues/494464740.
While the code here is not passed through build_webui() and therefore would not have our checks automatically applied to it, the desire to disallow 'any' is still applicable here.
Can you remove this? For example does the following work equivalently?
```suggestion
Object.assign(globalThis, {parseClientVariations, formatClientVariations});
```
"ignoreDeprecations": "6.0",Why is this necessary? Note that we are actively preporing the codebase to support v7, so this would be a blocker, see [1]. If you can't address in the same CL, can you file a bug to track this removal and add a TODO comment here?
import path from 'path';
import fs from 'fs';```suggestion
import path from 'node:path';
import fs from 'node:fs';
```
resolveId(source, importee) {{
if (source.includes('@bufbuild/protobuf/')) {{
const parts = source.split('@bufbuild/protobuf/');
const subpath = parts[parts.length - 1];
// Try direct path
const p = path.resolve('{node_modules_posix}', subpath);
if (fs.existsSync(p)) return p;
// Try as directory with index.js
const p_idx = path.resolve('{node_modules_posix}', subpath, 'index.js');
if (fs.existsSync(p_idx)) return p_idx;
// Try with .js extension
const p_js = path.resolve('{node_modules_posix}', subpath + '.js');
if (fs.existsSync(p_js)) return p_js;
}}
if (source === 'client_variations_proto') {{
return '{proto_js_posix}';
}}
return null;
}}Is inlining the entire Rollup plugin code here necessary? Can you move this to a checked-in rollup_plugin.js (or rollup_plugin.mjs) and instead trigger the plugin from here with the correct parameters?
See similar example from WebUI code generating rollup configs that invoke a plugin at [1] and the plugin itself at [2]. You can find generated configs in your out/<out>/<gen> folder, for example look at `out/gchrome/gen/chrome/browser/resources/tab_search/bundled/rollup.config.mjs`.
[1] https://source.chromium.org/search?q=rollup_plugin.mjs&ss=chromium%2Fchromium%2Fsrc:ui%2Fwebui%2Fresources%2Ftools%2F
[2] https://source.chromium.org/chromium/chromium/src/+/main:ui/webui/resources/tools/rollup_plugin.mjs
Related details
- Alexei Svitkine
- Andrzej Fiedukowicz
- Rebekah Potter
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Demetrios Papadopoulos (Gerrit)
Demetrios Papadopoulos added 1 comment![]( "Open in Gerrit")
"ignoreDeprecations": "6.0",Why is this necessary? Note that we are actively preporing the codebase to support v7, so this would be a blocker, see [1]. If you can't address in the same CL, can you file a bug to track this removal and add a TODO comment here?
Also see https://chromium-review.googlesource.com/c/chromium/src/+/7861311/ which is about to land and as of this writing we can build Win/Mac/Linux with v7 successfully. Would the CL here affect that?
Alexei Svitkine (Gerrit)
Alexei Svitkine added 4 comments![]( "Open in Gerrit")
(globalThis as any).parseClientVariations = parseClientVariations;
(globalThis as any).formatClientVariations = formatClientVariations;Are these global exports really needed? This file is already using JS modules and the `export` keyword. Can whoever uses these not `import` them directly from here?
Done, removed!
Demetrios PapadopoulosWhy is this necessary? Note that we are actively preporing the codebase to support v7, so this would be a blocker, see [1]. If you can't address in the same CL, can you file a bug to track this removal and add a TODO comment here?
Also see https://chromium-review.googlesource.com/c/chromium/src/+/7861311/ which is about to land and as of this writing we can build Win/Mac/Linux with v7 successfully. Would the CL here affect that?
Done, removed!
```suggestion
import path from 'node:path';
import fs from 'node:fs';
```
Done. Fixed in the standalone file.
resolveId(source, importee) {{
if (source.includes('@bufbuild/protobuf/')) {{
const parts = source.split('@bufbuild/protobuf/');
const subpath = parts[parts.length - 1];
// Try direct path
const p = path.resolve('{node_modules_posix}', subpath);
if (fs.existsSync(p)) return p;
// Try as directory with index.js
const p_idx = path.resolve('{node_modules_posix}', subpath, 'index.js');
if (fs.existsSync(p_idx)) return p_idx;
// Try with .js extension
const p_js = path.resolve('{node_modules_posix}', subpath + '.js');
if (fs.existsSync(p_js)) return p_js;
}}
if (source === 'client_variations_proto') {{
return '{proto_js_posix}';
}}
return null;
}}Is inlining the entire Rollup plugin code here necessary? Can you move this to a checked-in rollup_plugin.js (or rollup_plugin.mjs) and instead trigger the plugin from here with the correct parameters?
See similar example from WebUI code generating rollup configs that invoke a plugin at [1] and the plugin itself at [2]. You can find generated configs in your out/<out>/<gen> folder, for example look at `out/gchrome/gen/chrome/browser/resources/tab_search/bundled/rollup.config.mjs`.
[1] https://source.chromium.org/search?q=rollup_plugin.mjs&ss=chromium%2Fchromium%2Fsrc:ui%2Fwebui%2Fresources%2Ftools%2F
[2] https://source.chromium.org/chromium/chromium/src/+/main:ui/webui/resources/tools/rollup_plugin.mjs
Done, moved to a checked in file.
Note: The rollup configuration boilerplate is still kept here (consistently with some other Chromium examples like ui/webui/resources/tools/bundle_js.py) because the configuration must point to absolute paths in the build directory or temporary directories that aren't known until execution time.
Related details
- Andrzej Fiedukowicz
- Demetrios Papadopoulos
- Rebekah Potter
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Demetrios Papadopoulos (Gerrit)
Demetrios Papadopoulos voted and added 6 comments![]( "Open in Gerrit")
Votes added by Demetrios Papadopoulos
| Code-Review | +1 |
6 comments
LGTM with nits. Thanks for removing one more usage of Closure!
const subpath = parts[parts.length - 1];Nit: You can get the last item of an array as follows.
```suggestion
const subpath = parts.at(-1);
```
if (fs.existsSync(p)) return p;Per styleguide.
```suggestion
if (fs.existsSync(p)) {
return p;
}
```
(here and elsewhere)
const p_idx = path.resolve(nodeModulesPath, subpath, 'index.js');Per JS styleguide, these should be named with camelCase.
```suggestion
const pIdx = path.resolve(nodeModulesPath, subpath, 'index.js');
```
(here anh elsewhere in this file)
if (source === 'client_variations_proto') {
return protoJsPath;
}
return null;Nit: Use ternary.
```suggestion
return source === 'client_variations_proto' ? protoJsPath : null;
```
CWD = os.path.dirname(__file__)This is confusing. CWD stands for "current working directory" but this not the current working directory, which in python can be fetched via `os.getcwd()`.
Can you rename to `HERE_DIR` instead? This is a much more common naming for this variable, see examples below.
Related details
- Alexei Svitkine
- Andrzej Fiedukowicz
- Rebekah Potter
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexei Svitkine (Gerrit)
Alexei Svitkine added 6 comments![]( "Open in Gerrit")
(globalThis as any).parseClientVariations = parseClientVariations;
(globalThis as any).formatClientVariations = formatClientVariations;We are actively trying to ban 'any' across the codebase, see https://issues.chromium.org/issues/494464740.
While the code here is not passed through build_webui() and therefore would not have our checks automatically applied to it, the desire to disallow 'any' is still applicable here.
Can you remove this? For example does the following work equivalently?
```suggestionObject.assign(globalThis, {parseClientVariations, formatClientVariations});
```
Done
Nit: You can get the last item of an array as follows.
```suggestion
const subpath = parts.at(-1);
```
Done
Per styleguide.
```suggestion
if (fs.existsSync(p)) {
return p;
}
```
(here and elsewhere)
Done
const p_idx = path.resolve(nodeModulesPath, subpath, 'index.js');Per JS styleguide, these should be named with camelCase.
```suggestion
const pIdx = path.resolve(nodeModulesPath, subpath, 'index.js');
```
(here anh elsewhere in this file)
Done
if (source === 'client_variations_proto') {
return protoJsPath;
}
return null;Nit: Use ternary.
```suggestion
return source === 'client_variations_proto' ? protoJsPath : null;
```
Done
This is confusing. CWD stands for "current working directory" but this not the current working directory, which in python can be fetched via `os.getcwd()`.
Can you rename to `HERE_DIR` instead? This is a much more common naming for this variable, see examples below.
- Andrzej Fiedukowicz
- Rebekah Potter
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrzej Fiedukowicz (Gerrit)
Andrzej Fiedukowicz added 6 comments![]( "Open in Gerrit")
Looks good, but it's a bit of an obscure corner of the codebase for me and I don't think I fully understand it, so on high level two questions:
1. Have we tested that this still works? How?
2. Could we add README.md which provides with the general context for this tool and its purpose.
* @param data The serialized ClientVariations proto contents, e.g.serialized as base64?
// Nothing to do here -- it's fine to leave `decoded` empty if base64
// decoding fails.Can we just do:
```
return {
variationIds: [],
triggerVariationIds: [],
};
```
So that the reader doesn't have to analyze the whole code below in the corner case of empty string?
parsed = {variationId: [], triggerVariationId: []};ditto, maybe:
```
return {
variationIds: [],
triggerVariationIds: [],
};
```
export function variationsResolver(nodeModulesPath, protoJsPath) {Is it worth adding some context in the comment here? This is a bit cryptic and seems like is just trying various setups so it works in different contexts, but I might be misinterpreting that.
// Copyright 2020 The Chromium AuthorsShould be 2026?
Related details
- Alexei Svitkine
- Rebekah Potter
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexei Svitkine (Gerrit)
Alexei Svitkine added 6 comments![]( "Open in Gerrit")
Looks good, but it's a bit of an obscure corner of the codebase for me and I don't think I fully understand it, so on high level two questions:
1. Have we tested that this still works? How?
2. Could we add README.md which provides with the general context for this tool and its purpose.
Done on both. For 1, I added the following to the cl desc:
"Tested locally by building Chrome with this DevTools CL applied:
https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/7863572
... and navigating to a google.com url and inspecting headers in
dev tools.
"
* @param data The serialized ClientVariations proto contents, e.g.Alexei Svitkineserialized as base64?
Done
// Nothing to do here -- it's fine to leave `decoded` empty if base64
// decoding fails.Can we just do:
```
return {
variationIds: [],
triggerVariationIds: [],
};
```So that the reader doesn't have to analyze the whole code below in the corner case of empty string?
Done
parsed = {variationId: [], triggerVariationId: []};ditto, maybe:
```
return {
variationIds: [],
triggerVariationIds: [],
};
```
Done
export function variationsResolver(nodeModulesPath, protoJsPath) {Is it worth adding some context in the comment here? This is a bit cryptic and seems like is just trying various setups so it works in different contexts, but I might be misinterpreting that.
Done
// Copyright 2020 The Chromium AuthorsAlexei SvitkineShould be 2026?
Kept as-is. 2020 is correct because a) for existing files (this py file) we shouldn't bump copyright date and b) for code generated by files, the generated code should have matching copyright date to the generating file.
Related details
- Andrzej Fiedukowicz
- Demetrios Papadopoulos
- Rebekah Potter
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexei Svitkine (Gerrit)
Alexei Svitkine voted and added 1 comment![]( "Open in Gerrit")
Votes added by Alexei Svitkine
| Commit-Queue | +1 |
1 comment
Alexei Svitkine (Gerrit)
Alexei Svitkine added 1 comment![]( "Open in Gerrit")
Related details
- Andrzej Fiedukowicz
- Demetrios Papadopoulos
- Nico Weber
- Rebekah Potter
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Demetrios Papadopoulos (Gerrit)
Demetrios Papadopoulos voted and added 2 comments![]( "Open in Gerrit")
Votes added by Demetrios Papadopoulos
| Code-Review | +1 |
2 comments
Re-LGTM
if (!data) {Are you chaecking against the empty string here? If so can you make it explicit? Otherwise this should never be null/undefined based on the type at line 21.
Explicitly checking for the empty string makes it clearer whether this check is justified or if the type at line 21 is inaccurate, in which case it should be changed to be accurate.
Related details
- Alexei Svitkine
- Andrzej Fiedukowicz
- Nico Weber
- Rebekah Potter
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexei Svitkine (Gerrit)
Alexei Svitkine added 1 comment![]( "Open in Gerrit")
Are you chaecking against the empty string here? If so can you make it explicit? Otherwise this should never be null/undefined based on the type at line 21.
Explicitly checking for the empty string makes it clearer whether this check is justified or if the type at line 21 is inaccurate, in which case it should be changed to be accurate.
- Andrzej Fiedukowicz
- Nico Weber
- Rebekah Potter
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |