-
Notifications
You must be signed in to change notification settings - Fork 9.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
misc: refactor collect-strings.js to use glob and work on windows #9406
Changes from 7 commits
70088e1
5940f4f
5b7f6ac
aa2de69
8530cc8
2986d64
6d30674
3d6b326
c55a900
8d74620
8ef266e
3b778bf
baf44d7
726af3b
8cb8b59
3e3a5f5
c33a1ac
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,6 +9,7 @@ | |
/* eslint-disable no-console, max-len */ | ||
|
||
const fs = require('fs'); | ||
const glob = require('glob'); | ||
const path = require('path'); | ||
const esprima = require('esprima'); | ||
|
||
|
@@ -22,12 +23,12 @@ const UISTRINGS_REGEX = /UIStrings = (.|\s)*?\};\n/im; | |
*/ | ||
|
||
const ignoredPathComponents = [ | ||
'/.git', | ||
'/scripts', | ||
'/node_modules', | ||
'/test/', | ||
'-test.js', | ||
'-renderer.js', | ||
'**/.git/**', | ||
'**/scripts/**', | ||
'**/node_modules/**', | ||
'**/test/**', | ||
'**/*-test.js', | ||
'**/*-renderer.js', | ||
]; | ||
|
||
// @ts-ignore - @types/esprima lacks all of these | ||
|
@@ -44,52 +45,51 @@ function computeDescription(ast, property, startRange) { | |
|
||
/** | ||
* @param {string} dir | ||
* @param {Record<string, ICUMessageDefn>} strings | ||
* @return {Record<string, ICUMessageDefn>} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you revert the changes to the param/return type (here and for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. let's just chill this until after placeholders land |
||
*/ | ||
function collectAllStringsInDir(dir, strings = {}) { | ||
for (const name of fs.readdirSync(dir)) { | ||
const fullPath = path.join(dir, name); | ||
const relativePath = path.relative(LH_ROOT, fullPath); | ||
if (ignoredPathComponents.some(p => fullPath.includes(p))) continue; | ||
|
||
if (fs.statSync(fullPath).isDirectory()) { | ||
collectAllStringsInDir(fullPath, strings); | ||
} else { | ||
if (name.endsWith('.js')) { | ||
if (!process.env.CI) console.log('Collecting from', relativePath); | ||
const content = fs.readFileSync(fullPath, 'utf8'); | ||
const exportVars = require(fullPath); | ||
const regexMatches = !!UISTRINGS_REGEX.test(content); | ||
const exportsUIStrings = !!exportVars.UIStrings; | ||
if (!regexMatches && !exportsUIStrings) continue; | ||
|
||
if (regexMatches && !exportsUIStrings) { | ||
throw new Error('UIStrings defined but not exported'); | ||
} | ||
|
||
if (exportsUIStrings && !regexMatches) { | ||
throw new Error('UIStrings exported but no definition found'); | ||
} | ||
|
||
// @ts-ignore regex just matched | ||
const justUIStrings = 'const ' + content.match(UISTRINGS_REGEX)[0]; | ||
// just parse the UIStrings substring to avoid ES version issues, save time, etc | ||
// @ts-ignore - esprima's type definition is supremely lacking | ||
const ast = esprima.parse(justUIStrings, {comment: true, range: true}); | ||
|
||
for (const stmt of ast.body) { | ||
if (stmt.type !== 'VariableDeclaration') continue; | ||
if (stmt.declarations[0].id.name !== 'UIStrings') continue; | ||
|
||
let lastPropertyEndIndex = 0; | ||
for (const property of stmt.declarations[0].init.properties) { | ||
const key = property.key.name; | ||
const message = exportVars.UIStrings[key]; | ||
const description = computeDescription(ast, property, lastPropertyEndIndex); | ||
strings[`${relativePath} | ${key}`] = {message, description}; | ||
lastPropertyEndIndex = property.range[1]; | ||
} | ||
} | ||
function collectAllStringsInDir(dir) { | ||
/** @type {Record<string, ICUMessageDefn>} */ | ||
const strings = {}; | ||
|
||
const files = glob.sync(path.relative(LH_ROOT, dir) + '/**/*.js', { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. comment about glob only accepting There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Evidently, glob doesn't only accept There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we 100% positive it's working as expected... https://github.com/isaacs/node-glob#windows is so convincing that only There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. anyway, I have empirical evidence that it works .... on my one Windows machine ;) Running collect strings resulted in no unstaged changes. Got all the expected output. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. apparently this is a whole thing: isaacs/minimatch#109, isaacs/node-glob#212 🤷♂ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. aight, passing these to path.join normalizes to the OS's file sep. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Which despite what the docs of node-glob state, it does seem to then convert to AFAICT :) |
||
cwd: LH_ROOT, | ||
ignore: ignoredPathComponents, | ||
}); | ||
for (const relativePathToRoot of files) { | ||
const absolutePath = path.join(LH_ROOT, relativePathToRoot); | ||
if (!process.env.CI) console.log('Collecting from', relativePathToRoot); | ||
|
||
const content = fs.readFileSync(absolutePath, 'utf8'); | ||
const exportVars = require(absolutePath); | ||
const regexMatches = !!UISTRINGS_REGEX.test(content); | ||
const exportsUIStrings = !!exportVars.UIStrings; | ||
if (!regexMatches && !exportsUIStrings) continue; | ||
|
||
if (regexMatches && !exportsUIStrings) { | ||
throw new Error('UIStrings defined but not exported'); | ||
} | ||
|
||
if (exportsUIStrings && !regexMatches) { | ||
throw new Error('UIStrings exported but no definition found'); | ||
} | ||
|
||
// @ts-ignore regex just matched | ||
const justUIStrings = 'const ' + content.match(UISTRINGS_REGEX)[0]; | ||
// just parse the UIStrings substring to avoid ES version issues, save time, etc | ||
// @ts-ignore - esprima's type definition is supremely lacking | ||
const ast = esprima.parse(justUIStrings, {comment: true, range: true}); | ||
|
||
for (const stmt of ast.body) { | ||
if (stmt.type !== 'VariableDeclaration') continue; | ||
if (stmt.declarations[0].id.name !== 'UIStrings') continue; | ||
|
||
let lastPropertyEndIndex = 0; | ||
for (const property of stmt.declarations[0].init.properties) { | ||
const key = property.key.name; | ||
const message = exportVars.UIStrings[key]; | ||
const description = computeDescription(ast, property, lastPropertyEndIndex); | ||
strings[`${relativePathToRoot} | ${key}`] = {message, description}; | ||
lastPropertyEndIndex = property.range[1]; | ||
} | ||
} | ||
} | ||
|
@@ -99,6 +99,7 @@ function collectAllStringsInDir(dir, strings = {}) { | |
|
||
/** | ||
* @param {Record<string, ICUMessageDefn>} strings | ||
* @return {Record<string, ICUMessageDefn>} | ||
*/ | ||
function createPsuedoLocaleStrings(strings) { | ||
/** @type {Record<string, ICUMessageDefn>} */ | ||
|
@@ -152,13 +153,18 @@ function writeStringsToLocaleFormat(locale, strings) { | |
fs.writeFileSync(fullPath, JSON.stringify(output, null, 2) + '\n'); | ||
} | ||
|
||
const strings = collectAllStringsInDir(path.join(LH_ROOT, 'lighthouse-core')); | ||
const psuedoLocalizedStrings = createPsuedoLocaleStrings(strings); | ||
const coreStrings = collectAllStringsInDir(path.join(LH_ROOT, 'lighthouse-core')); | ||
console.log('Collected from LH core!'); | ||
|
||
collectAllStringsInDir(path.join(LH_ROOT, 'stack-packs/packs'), strings); | ||
const stackPackStrings = collectAllStringsInDir(path.join(LH_ROOT, 'stack-packs/packs')); | ||
console.log('Collected from Stack Packs!'); | ||
|
||
const strings = { | ||
...coreStrings, | ||
...stackPackStrings, | ||
}; | ||
|
||
const psuedoLocalizedStrings = createPsuedoLocaleStrings(strings); | ||
writeStringsToLocaleFormat('en-US', strings); | ||
writeStringsToLocaleFormat('locales/en-XL', psuedoLocalizedStrings); | ||
console.log('Written to disk!'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These weren't being collected for en-XL before.