chore: snapshot types that we want to not accidentally deprecate or remove #1705
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR introduces type snapshot testing to prevent accidental breaking changes to public TypeScript interfaces, using the TypeScript Compiler API to extract and validate type definitions.
- Added
src/__tests__/config-snapshot.test.tsto snapshot test critical interfaces likePostHogConfig,AutocaptureConfig, andSessionRecordingOptions - Implemented type extraction using TypeScript's Compiler API to convert interface definitions into serializable JSON structures
- Test focuses on three key configuration interfaces that are most important for external consumers
- Type extraction logic could be enhanced to better handle complex types like unions and intersections
1 file(s) reviewed, 4 comment(s)
Edit PR Review Bot Settings | Greptile
| import ts from 'typescript' | ||
|
|
||
| function extractTypeInfo(filePath: string, typeName: string): string { | ||
| const program = ts.createProgram([filePath], {}) |
There was a problem hiding this comment.
logic: createProgram() without compiler options may miss important type information. Should include at least noImplicitAny and strictNullChecks
| function processType(type: ts.Type): any { | ||
| if (type.isUnion() || type.isIntersection()) { | ||
| return type.types.map(getTypeString) | ||
| } else if (type.isClassOrInterface()) { | ||
| const result: Record<string, any> = {} | ||
| type.getProperties().forEach((symbol) => { | ||
| const propType = checker.getTypeOfSymbol(symbol) | ||
| result[symbol.getName()] = processType(propType) | ||
| }) | ||
| return result | ||
| } else if (type.symbol && type.symbol.valueDeclaration) { | ||
| return getTypeString(type) | ||
| } | ||
| return getTypeString(type) | ||
| } |
There was a problem hiding this comment.
logic: processType() doesn't handle type aliases, enums, or literal types correctly. These types will be stringified instead of preserving their structure
There was a problem hiding this comment.
This would be good to fix :)
| ts.forEachChild(sourceFile, (node) => { | ||
| if (ts.isInterfaceDeclaration(node) && node.name.text === typeName) { | ||
| const type = checker.getTypeAtLocation(node) | ||
| result = processType(type) | ||
| } | ||
| }) |
There was a problem hiding this comment.
style: forEachChild only processes the first matching interface. If there are multiple interfaces with the same name in different namespaces/modules, this could miss them
There was a problem hiding this comment.
I'm glad I'm as smart as an AI, I noticed the same. I don't think this is ever going to happen - it shouldn't - so I'm fine to keep this as is
|
Size Change: 0 B Total Size: 3.28 MB ℹ️ View Unchanged
|
There was a problem hiding this comment.
That's very cool, thank you for looking into it!
I'd love if the snapshot file was slightly more readable.
This one is just ridiculous, for example
I think one of my suggestions will improve this slightly by removing the infinitely-many escapeslashes we have for nested types
| } else if (type.symbol && type.symbol.valueDeclaration) { | ||
| return getTypeString(type) | ||
| } | ||
| return getTypeString(type) |
There was a problem hiding this comment.
Last else isn't needed, we're just doing the same as the default clause below
|
|
||
| function processType(type: ts.Type): any { | ||
| if (type.isUnion() || type.isIntersection()) { | ||
| return type.types.map(getTypeString) |
There was a problem hiding this comment.
Rather than calling getTypeString we should processType recursively, this will allow us to capture nested unions like {a: {b: number}} | {a: {b: string}}
There was a problem hiding this comment.
yep, and that picked up things like AutocaptureConfig without them needing a separate test 👍
| ts.forEachChild(sourceFile, (node) => { | ||
| if (ts.isInterfaceDeclaration(node) && node.name.text === typeName) { | ||
| const type = checker.getTypeAtLocation(node) | ||
| result = processType(type) | ||
| } | ||
| }) |
There was a problem hiding this comment.
I'm glad I'm as smart as an AI, I noticed the same. I don't think this is ever going to happen - it shouldn't - so I'm fine to keep this as is
We don't need to look inside `Regexp`, it's fine to render it as is
we accidentally removed some options from a config in another PR
computers should protect us
this is 99% ChatGPT so review accordingly 🙈
this converts provided types into a dict and snapshots that
this should let us known when we're accidentally breaking types for consumers