-
Notifications
You must be signed in to change notification settings - Fork 28
[code-infra] Add command run duration measurement #687
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
base: master
Are you sure you want to change the base?
Conversation
Bundle size report
|
|
|
||
| await Promise.all(promises); | ||
| console.log( | ||
| `✅ Built "${bundle}" with babel in ${(measureFn(markLabel).duration / 1000).toFixed(3)}s.`, |
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.
Handy to see when comparing the new build tools transpilation times.
| * @typedef {import('yargs').CommandModule<{}, any>} CommandModule | ||
| */ | ||
|
|
||
| const commands = { |
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.
Not a huge fan of this style, it's such an arbitrary way of separating the different commands, and it makes the logic in this file hard to understand.
How do you feel about instead we create a function that we can use to wrap a command handler function and just in each command file that we want this output on we wrap their handler function?
import withCmdMeasure from './withCmdMeasure'
export default {
command: 'argos-push',
handler: withCmdMeasure(async (argv) => {
// ...
})
}I suppose in the wrapper we can read the command name from argv._[o].
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 would also work. I don't have during opinions here.
This is something I thought of previously.
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.
I think the way it's currently done is a bit of an anti-pattern.
4caaa08 to
08ca380
Compare
08ca380 to
618b64d
Compare
618b64d to
2e2a631
Compare
2e2a631 to
d89e8c0
Compare
d89e8c0 to
a00de1b
Compare
to each relevant sub-command. Removed markFn and measureFn functions and wrapped their functionality in a single HOC `withPerformanceMeasurement`.
a00de1b to
861ccff
Compare
| } | ||
|
|
||
| export default /** @type {import('yargs').CommandModule<{}, Args>} */ ({ | ||
| const command = /** @type {import('yargs').CommandModule<{}, Args>} */ ({ |
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.
| const command = /** @type {import('yargs').CommandModule<{}, Args>} */ ({ | |
| export default /** @type {import('yargs').CommandModule<{}, Args>} */ ({ |
I really would like to keep this simple and have a function that only wraps the handler, something we can add/remove like a middleware without imposing changes on how these command modules are structured. It's not important enough as a feature to be so visually present in our source code.
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.
The problem with handler: withPerformanceMeasurement(label, async (args) => {}) is that args's type does not get detected automatically the way it is when its just handler: async (args) => {}.
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.
I believe this should do the trick
// ./types.d.ts
import { CommandModule } from 'yargs';
export type CommandHandler<T> = CommandModule<{}, T>['handler'];
export type HandlerMiddleware = <T>(handler: CommandHandler<T>) => CommandHandler<T>;// ./myCommand.mjs
/**
* @type {import('./types').HandlerMiddleware}
*/
function withMiddleware(handler) {
return async (argv) => {
await handler(argv);
};
}
// ...
handler: withMiddleware(async (argv) => {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.
I'll pick this up again later. Not happy with the type inference that I've added.
|
This pull request has been inactive for 30 days. Please remove the stale label or leave a comment to keep it open. Otherwise, it will be closed in 15 days. |
| performance.clearMarks(startMark); | ||
| performance.clearMarks(endMark); | ||
| performance.clearMeasures(label); |
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.
Why clear the marks? Isn't it helpful when profiling?
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.
lastMeasurement already gets the measured value. So clearing after usage. These scripts are not really meant to be profiled to be studied for tiny perf improvements. So clearing it.
to each relevant sub-command. And some cleanup.
Removed
copy-filescommand since all repos are now migrated.Removed markFn and measureFn functions and wrapped their functionality
in a single HOC
withPerformanceMeasurement.