Skip to content
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

feat: extract & expose toString for stringifying types #1216

Closed
wants to merge 4 commits into from

Conversation

haltcase
Copy link

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!

Copy link
Collaborator

@Gerrit0 Gerrit0 left a 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)) {
Copy link
Collaborator

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.

Copy link
Author

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.

src/test/stringifiers.test.ts Outdated Show resolved Hide resolved
@@ -0,0 +1,151 @@
import * as t from './models/types';
Copy link
Collaborator

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.

Copy link
Author

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.

src/index.ts Outdated Show resolved Hide resolved
src/lib/models/types/array.ts Show resolved Hide resolved
@Gerrit0
Copy link
Collaborator

Gerrit0 commented Jun 28, 2020

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.

@haltcase
Copy link
Author

@Gerrit0 cool, so your library-mode branch subsumes the functionality of this PR? If that's the case, I'd be up for working on JSON deserialization once the dust settles. Is that something you consider in scope for the type models in typedoc?

@Gerrit0
Copy link
Collaborator

Gerrit0 commented Jun 30, 2020

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 :)

@Gerrit0
Copy link
Collaborator

Gerrit0 commented Sep 4, 2023

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..

@Gerrit0 Gerrit0 closed this Sep 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RFC] Consider extracting toString functionality to standalone module
2 participants