-
-
Notifications
You must be signed in to change notification settings - Fork 734
feat: extract & expose toString
for stringifying types
#1216
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
Conversation
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.
Looking good! A few comments on what's been done so far.
* Return a string representation of the given type. | ||
*/ | ||
export const stringifyType = (node: t.Type): string => { | ||
if (!node || !node.type || !{}.hasOwnProperty.call(stringifiers, node.type)) { |
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.
We control stringifiers
, no need for the paranoid version of hasOwnProperty. Also, I'd rather not worry about not being provided node/node.type. This is a TS only library with strictNullChecks on, if someone breaks that contract, that's on them.
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.
Mostly I did this so that we could better support passing in an object pulled from typedoc JSON, given the lack of deserializing (related: #1180) and my use cases in the RFC. In that case it would probably be better to use an alternative type that ensures we only use properties that exist on both the JSON objects and the live models.
@@ -0,0 +1,151 @@ | |||
import * as t from './models/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.
So this isn't your fault, it was a problem with the existing code but nobody ran into it since it wasn't used externally... But if we are going to more publicly expose this functionality we need to fix this. The serializers can generate incorrect types. Consider this:
const broken = new t.IntersectionType([
new t.ReferenceType('T', t.ReferenceType.SYMBOL_FQN_RESOLVED),
new t.UnionType([
new t.ReferenceType('U', t.ReferenceType.SYMBOL_FQN_RESOLVED),
new t.ReferenceType('V', t.ReferenceType.SYMBOL_FQN_RESOLVED)
])
])
This will currently stringify to T & U | V
while the actual type is T & (U | V)
There's also a problem with function types and conditional types. () => any extends 1 ? 1 : 2
is different than (() => any) extends 1 ? 1 : 2
.
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.
Agreed, I did notice there could be some precedence issues with complex union/intersection types. We'll have to add some parenthesizing logic.
Heads up - as a part of library mode work I think I've resolved the issues with wrapping. https://github.com/TypeStrong/typedoc/blob/library-mode/src/lib/models/types/abstract.ts#L59 This still won't quite work for your JSON use case, but library mode also brings us much closer to being able to reinflate JSON into the original models. |
@Gerrit0 cool, so your |
It doesn't contain everything that this does insofar as it won't work for JSON models... I think deserialization will meet all the requirements from here. I absolutely want to support deserialization of the full project. Tracking in #910 :) |
At this point, I don't think I'm going to ever merge this -- it's a nice idea, but until TypeDoc itself can be run without Node APIs, I don't want to maintain multiple entry points to typedoc. TypeDoc 0.24 did add support for deserializing a JSON project, so at this point I'm recommending using TypeDoc's deserialization API, and then calling toString on the created type.. |
Closes #1160
This essentially moves all the type model
toString
implementations into a separate module and exposes them for users, as spec'd in the above issue.toString
is implemented on the base model now, so that method is still kept around for backward compatibility currently.Currently it should have parity, and has a start for stringifying reflections in a similar manner (just call signature for now).
I've also added tests, which don't cover everything, but these weren't actually tested before at all. Progress!