-
Notifications
You must be signed in to change notification settings - Fork 27
[code-infra] Add cli to extract error codes independent of build #641
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
Conversation
Bundle size report
|
1a309e5
to
dfc7c78
Compare
dfc7c78
to
648fb01
Compare
648fb01
to
0a88db4
Compare
0a88db4
to
4dc4a1c
Compare
d9144f2
to
4a7d09e
Compare
4a7d09e
to
405bc3a
Compare
88b4f5c
to
38e8bf2
Compare
38e8bf2
to
a3e11b0
Compare
a3e11b0
to
2690021
Compare
2690021
to
8f6d918
Compare
* @param {'all' | 'allSettled'} [options.promiseMethod='all'] | ||
* @returns {Promise<void>} | ||
*/ | ||
export async function wrapInWorker(fn, options) { |
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.
Suggestion to simplify this helper a bit:
async function mapConcurrently<T, R>(items: T[], mapper: (t: T) => Promise<R>, concurrency: number): Promise<R[]>
- Make the signature more like a map operation, allow returning results, to satisfy more use-cases.
- Remove the
promiseMethod
, this can just be done in themapper
by the consumer. We can avoid leaking the implementation this way. - Make
concurrency
explicitly required, avoid consumers to rely on a arbitrary default.
workers.push(worker); | ||
} | ||
|
||
await Promise.allSettled(workers); |
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.
Seems like allSettled
wasn't wanted here, the JSON.parse
is already wrapped, and the fs.readFile
we would want to propagate its errors.
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.
My thinking was that we'd want to know about all the erroneous json files in the same run instead of just failing on the first wrong file.
This way, devs can fix the files together instead of running the command multiple times.
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.
That would still work, the JSON.parse
is wrapped in try
/catch
already, and the catch
just logs. as far as I can see, the allSettled
is not doing anything, except from hiding errors from fs.readFile
, which is undesired.
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.
Yeah. I figured later. Removed the option.
* @param {Function} fn - The function to measure. | ||
* @returns {Promise<number>} - The duration of the function execution in milliseconds. | ||
*/ | ||
export async function measurePerf(label, fn) { |
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.
Poor man's tracer 😄 . Suggestion: could be more versatile if we maintain return value of the function, and instead
async function markFn<F extends Function>(label: string, fn: (...args: Parameters<F>)): Awaitable<ReturnValue<F>>;
// and
async function measureFn(label: string)
// then use as
const result = await markFn('foo', async () => {
return // ...
});
console.log(`It took ${measureFn('foo').duration}ms to calculate ${result}`)
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.
Calling markFn('foo',...)
and then again measureFn('foo')
seems repititive. Should be another way to do both in the same call.
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.
It's sort of repetitive, but ideally we make it so we can wrap existing code with measureFn
, including code that returns values. I think it would be nice to just simply complement the performance.mark
and performance.measure
functions with functions that can automatically wrap and keep the same API philosophy of being able to just insert some function calls without semantically having to change the program.
Doesn't matter much, not a blocker for this PR at all
return shouldNotSkip; | ||
}, | ||
); | ||
await Promise.all(packages.map((pkg) => extractErrorCodesForPackage(pkg, errors, detection))); |
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.
Would it make sense to determine the full list of files across all packages, and then run the extractor concurrently over that? Otherwise we multiply the concurrency with each specified package. i.e. 1 package => concurrency 30, 3 packages => concurrency becomes 90. So we then would have concurrency of 90, but it wouldn't be fully used all the time if there's e.g. an asymmetrically large package in there.
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.
Yeah. That's better. I was working on the pretext of packages but your suggestion makes sense.
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.
Love this, made a few suggestions, but no blockers for me.
8f6d918
to
7ac74c7
Compare
7ac74c7
to
7f98435
Compare
7f98435
to
30e7ab7
Compare
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.
👌
This exposes a cli command
extract-error-codes
to extract annotated errors from workspace packages. This makes the error code extraction independent of the build process. The command is supposed to work on the whole workspace instead of at the individual package level (unless we have a use case later).There's a way to make this even faster by just processing the files that have changed in this branch instead of all the files in all the packages. But the current gains are already huge and using git would be a micro-optimization. We could revisit this in future.
Also created a helper
wrapInWorker
function to abstract creation of worker pools and migrated code to use it.Closes #580
Repo PRs -