add BrowserRun types#6771
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
6490581 to
a78db9a
Compare
|
/bonk review this PR |
There was a problem hiding this comment.
Review: BrowserRun types
Good addition overall — the types are well-documented with JSDoc, follow the BrowserRun product prefix convention correctly, and the overloaded quickAction pattern is a solid approach for discriminated-union-style APIs.
I have a few issues to flag, one of which is a type system anti-pattern explicitly called out in this repo's guidelines.
Summary of issues
result: objectis a repo anti-pattern —types/AGENTS.mdsays "Never use bareobjectas a type."BrowserRunJsonOptionsunion doesn't enforce the documented constraint — bothpromptandresponse_formatcan be provided in either branch.cacheTTLplacement — @ruifigueira already flagged this; needs confirmation.generated-snapshot/files should be regenerated, not hand-edited —types/AGENTS.mdstates: "Do not edit files ingenerated-snapshot/directly — they are overwritten byjust generate-types." Please runjust generate-typesand commit the resulting files instead of manually editing the snapshots.- Minor: inconsistent
metapresence across response types (may be intentional, but worth confirming). - Minor:
fetch()method has no JSDoc.
|
Review posted successfully. Here's a summary of what I flagged: Review posted on PR #67716 issues identified, 5 inline comments:
|
a78db9a to
13f31f1
Compare
|
Please can you add comments when marking conversations as resolved. Otherwise it's difficult to know what the resolution was. |
re-added them. Didn't realize that clicking Resolve doesn't automatically save the comment that I typed :) |
jamesopstad
left a comment
There was a problem hiding this comment.
The snapshot needs updating but otherwise looks good.
13f31f1 to
e49c8ce
Compare
Updated, can you re-approve please? |
Summary
Adds TypeScript type definitions for the existing
browserbinding: