-
Notifications
You must be signed in to change notification settings - Fork 4
Adds --json and other params #6
base: main
Are you sure you want to change the base?
Conversation
I threw in some vendored files to make it easier to test too, they're npmignored |
TODO: calling |
const args: string[] = [] | ||
const opts: Opts = {} | ||
|
||
let foundOpt: string | undefined = undefined |
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.
Feel free to just pull in yargs. It's already there indirectly.
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 forgot to come back, doh - OK, I'm good for this
} | ||
}); | ||
|
||
if (args.length !== 3 && args.length != 4) { |
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 suspect you'll need to update these length checks.
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.
yargs'll fix that
const thresholdDuration = 5E5; // microseconds | ||
const minDuration = 1E5; // microseconds | ||
const minPercentage = 0.6; | ||
const thresholdDuration = Number(opts.thresholdDuration) || 5E5; // microseconds |
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.
Ignorant question: is this different from +opts.thresholdDuration
? I've been using that.
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.
No, basically the exact same, I just find Number to feel more explicit
await makeHotStacks(root); | ||
} | ||
|
||
type TreeNode = { |
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.
interface
?
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.
Oh, maybe that doesn't work with recursive types?
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.
interface will be fine, they can recurse too
const path = require("path"); | ||
console.error(`Usage: ${path.basename(process.argv[0])} ${path.basename(process.argv[1])} trace_path [type_path]`); | ||
console.error(`Options: --json [path] Prints a JSON object of the results to stdout`); |
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.
Does it print to stdout or path
?
if (Object.entries(tree).length) { | ||
|
||
if (tree && Object.entries(tree).length) { | ||
if (opts.json) { |
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.
Intuitively, I would have expected json output to replace the normal output, not supplement it.
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.
You're not wrong to feel that, but I think there's still value in keeping the logs (for example, we can show the terminal output in a build log)
// Sort slow to fast | ||
let childTree = {}; | ||
|
||
async function makePrintableTree(curr: EventSpan, currentFile: string | undefined, positionMap: PositionMap): Promise<TreeNode | undefined> { |
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.
"Printable" was my word for "treeify-compatible", so "makeTree" is probably fine now.
treeNode.message = `Compare types ${args.sourceId} and ${args.targetId}`; | ||
treeNode.terseMessage = `Compare types ${args.sourceId} and ${args.targetId}`; | ||
// TODO: Add start and end links | ||
return |
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.
return treeNode
?
} | ||
} | ||
return undefined; | ||
} | ||
} | ||
|
||
function updateTypeTreePositions(typeTree: any): any { | ||
function updateTypeTreePositions(node: TreeNode, typeTree: any): any { |
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 is a TreeNode
required?
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.
Passing in a root node so it can recurse, maybe I should rename it to say it will recurse
// If it's going to include a JSON path, make it per-trace | ||
let jsonPath: string | undefined = undefined | ||
if (opts.includes("--json")) { | ||
const hash = crypto.createHash('sha256').update(project.tracePath).digest("hex"); |
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 are we hashing files? I don't think there's a way to update one trace out of a family - are we guarding against having to redo work on crash?
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.
Basically you ask the project "give me a JSON file at this path" - but to generate that each sub-project in the project will generate its own JSON file and these need unique names, so we take the path to the project and turn it into a SHA so they don't collide
--opt
flagsThe primary output asset is now a TreeNode, which is more editor focused, and from that we can derive a treeable tree for the same text output