Support for infer in template literals, intrinsic types and more
#2053
Conversation
Useful for extends types within conditional nodes
Added support for never types and template literal types for extends types
Useful for extends types within conditional nodes
Added support for returning intrinsic types for extends clauses. Also added support for Capitalize<string> expressions
…o isAssignableTo unit tests
… of conditional types
…onditionals with mapped type This type is the original type from our code which brought me onto this journey of adding support for more advanced inference in the first place. Thought it was good to have it here for reference in the future.
|
@arthurfiorette could you take a look at this? |
|
Whoa! Sorry for the 3 months delay :/ I just saw this PR... If this happens again, fell free to tag me multiple times XD |
There was a problem hiding this comment.
General implementation reviewed :)
Ik some breaking changes with past generations will happen but I'd a more detailed explanation of how the final output changed.
Unless I'm wrong here, just deleting older tests might not seem like the best approach, could you describe more about this as well?
@domoritz i'd love your help or from anyone else to review the tests results (main.ts vs schema.json) output in our tests to ensure generated json is the most correct for that situation.
|
|
||
| export const intrinsicMethods: Record<string, ((v: string) => string) | undefined> = { | ||
| export const intrinsicMethods = { |
There was a problem hiding this comment.
| export const intrinsicMethods = { | |
| export const intrinsicMethods: Record<string, (str: string) => string> = { |
There was a problem hiding this comment.
No need to use as const satisfies here: Type it directly and avoid the method access, using intrinsicMethods[methodName] will also be easier to read.
| }; | ||
| } as const satisfies Record<string, ((v: string) => string) | undefined>; | ||
|
|
||
| function isIntrinsicMethod(methodName: string): methodName is keyof typeof intrinsicMethods { |
There was a problem hiding this comment.
| function isIntrinsicMethod(methodName: string): methodName is keyof typeof intrinsicMethods { |
| const method = intrinsicMethods[methodName]; | ||
|
|
||
| if (!method) { | ||
| if (!isIntrinsicMethod(methodName)) { |
There was a problem hiding this comment.
| if (!isIntrinsicMethod(methodName)) { | |
| if (!intrinsicMethods[methodName]) { |
| const literals = extractLiterals(type); | ||
| expanded = expanded.flatMap((prefix) => literals.map((suffix) => prefix + suffix)); | ||
| } catch { | ||
| return new StringType(); |
There was a problem hiding this comment.
saw this in a lot of functions in your pr, catch-all might hide any kind of other errors that happen, please check errors into the ones you expect them and rethrow any others.
ik throwing errors is bad in general until we fix them but this only hides entirely any kind of faulty implementation that might be happening.
please first check that all your updated tests does not throws anything at all and those catches are only to handle future unknown behaviors.
| import type { BaseType } from "./BaseType.js"; | ||
| import { PrimitiveType } from "./PrimitiveType.js"; | ||
|
|
||
| export class IntrinsicType extends PrimitiveType { |
There was a problem hiding this comment.
The term "intrinsic" is not formally defined in the TypeScript documentation, would be good if you write a jsdoc saying that is for generic built in types like Uppercase and so on...
| @@ -182,11 +176,139 @@ export function isAssignableTo( | |||
|
|
|||
| // Check literal types | |||
| if (source instanceof LiteralType) { | |||
| if (target instanceof IntrinsicType) { | |||
There was a problem hiding this comment.
Ik object properties are a pain in the ass to handle and parse but most of this implementation is very specific to this use case and hard to maintain, please add more comments about why you are doing it that way.
| return isAssignableTo(part, new LiteralType(value), inferMap, insideTypes); | ||
| }; | ||
|
|
||
| for (const part of parts) { |
There was a problem hiding this comment.
what does this all do?
| return false; | ||
| } | ||
| } else if (type instanceof UnionType) { | ||
| const matchFound = type.getTypes().some((unionPart) => { |
| if (infer === undefined) { | ||
| inferMap.set(key, source); | ||
| } else { | ||
| inferMap.set(key, new UnionType([infer, source])); |
There was a problem hiding this comment.
check if infer is a union type and append to it instead of creating N levels of UnionType.
| return true; | ||
| } | ||
| } | ||
| current = current.parent; |
There was a problem hiding this comment.
you can rewrite this into a do {} while (current = current.parent) statement :)
|
Also, nice job @flugg for such high quality implementation :) |
Closes #1355
Adding full type inference for template literals were quite a lot more work than first anticipated, particularly because
TemplateLiteralNodeParserwould narrow the type to aLiteralType,StringTypeorUnionTypebefore sent toisAssignableToinConditionalNodeParser. My proposed solution is to add a check withinTemplateLiteralNodeParserto see if the node exists within an extends clause of a conditional, in which case we create a newTemplateLiteralTypewhere we retain the original information stored in the template literal, which allows us to segment through the parts and verify if the source is assignable.Additionally, I've done the same for
IntrinsicNodeParser, which had the same limitations, where bothStringTypeandInferTypeas an argument in a condiitonal type would fail.I've added extensive test cases for both.