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: support for sourceless nodes. #1386

Merged
merged 24 commits into from Oct 17, 2022
Merged

feat: support for sourceless nodes. #1386

merged 24 commits into from Oct 17, 2022

Conversation

arthurfiorette
Copy link
Collaborator

@arthurfiorette arthurfiorette commented Aug 28, 2022

Hey! I've been using your project to help with json schema generation for my recent kitajs project. Its being great, but ive encountered some problems with my specific scenarios.

This PR is what I've been fixing along the time. A shortly brief:

  • It currently does not have any specific unit tests, but its tested alongside with kitajs.
  • This "sourceless" requirement is because sometimes I have to create type nodes with typeChecker.typeToTypeNode, which creates nodes without a valid source code and -1 locations.
  • Also, I needed to generate references without the #/reference/ prefix, so I added another option too.
  • Promise types were being interpreted as a plain object without any fields, now it resolves to its generic parameter.

Version

Published prerelease version: v1.2.0-next.1

Changelog

🎉 This release contains work from new contributors! 🎉

Thanks for all your work!

❤️ Arthur Fiorette (@arthurfiorette)

❤️ Sean Keenan (@sean9keenan)

🚀 Enhancement

🐛 Bug Fix

🔩 Dependency Updates

Authors: 4

@domoritz
Copy link
Member

Thanks for the pull request. Please add tests and also fix the todo.

@arthurfiorette
Copy link
Collaborator Author

arthurfiorette commented Aug 28, 2022

@domoritz, for now, I don't have any spare time to to these tests, are you up to do it? They seem pretty small tests.

Furthermore, the TODO is more of a question than a task. I couldn't find any place where using the string "unresolved" could cause problems. And as you are more familiar with this lib, could this cause an issue?

@domoritz
Copy link
Member

I'm starting to teach on Tuesday so I unfortunately don't have cycles to write tests for you.

@domoritz domoritz changed the title Support for sourceless nodes and optional #/reference/ usage. feat: support for sourceless nodes and optional #/reference/ usage. Aug 29, 2022
@domoritz
Copy link
Member

Thanks for adding the tests. Ping me when they are passing and I will review the pull request.

@arthurfiorette
Copy link
Collaborator Author

@domoritz, I think that was it! Got my new notebook yesterday and now i can code while I'm at philosophy classes 😁.

The only thing remaining is this TODO. It is more a kind of a note (as I said before), and as all tests passed, seems just fine. But who knows all future cases.

Furthermore, the TODO is more of a question than a task. I couldn't find any place where using the string "unresolved" could cause problems. And as you are more familiar with this lib, could this cause an issue?

const file = !source
? // TODO: Use better filename for unknown files
"unresolved"

I'd would let it here just to our future self keep an eye on it.

@arthurfiorette
Copy link
Collaborator Author

@domoritz, Can this be released soon?

Copy link
Member

@domoritz domoritz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the pull request. I do really appreciate the time you put into making these changes and sending the pull request. Looking over the changes, I think this pull request does too many things, though.

In general, I want to avoid introducing new options especially when it's things that can easily be done by pos-processing the JSON or that are for a specific use case only. So in this case I don't think I want to include the change to make references optional. They are needed according to json schema spec, no?

What are sourceless types? If this is something common, I'd be happy to include the change as a separate pull request.

I'm happy to be convinced otherwise but I try to think about the future of the project and want to be thoughtful when adding features.

@arthurfiorette
Copy link
Collaborator Author

arthurfiorette commented Sep 5, 2022

What are sourceless types? If this is something common, I'd be happy to include the change as a separate pull request.

I'm calling sourceless nodes any ts.Node object that is created programatically. Like when using ts.typeToTypeNode or manually invoking ts.factory methods. For now, ts-json-schema-generator only works with ts.Nodes that are read from a real file in the machine, as it relies on node.getSourceFile() calls. The sourceless changes are to support any type of ts.Node, independently of where it was created.

@arthurfiorette
Copy link
Collaborator Author

arthurfiorette commented Sep 5, 2022

About the #/reference/ opt-out option, involves the "contradictory" usage of it. You can read more about it at json-schema-org/json-schema-spec#279 (comment).

And because of the above thread (any many others sub discussions spread out there), a lot of json schema related libraries are have different behaviours for #/reference/s, this includes fastify, avj and many others...

This option is just another related feature that is disabled by default, and it helps in these cases without having to maintain a fork of this project with their required changes, as it currently this project does not allow all nescessary customizations to make it work.

@arthurfiorette
Copy link
Collaborator Author

I agree with you that this PR has two unrelated to each other changes. I could split them into two small PRs, but that would lead to me just copying code over branches...

src/NodeParser/TypeReferenceNodeParser.ts Outdated Show resolved Hide resolved
src/NodeParser/TypeReferenceNodeParser.ts Outdated Show resolved Hide resolved
src/NodeParser/TypeReferenceNodeParser.ts Outdated Show resolved Hide resolved
@domoritz
Copy link
Member

domoritz commented Sep 6, 2022

I'm not convinced that I want to support optional references at this point. Can you remove it from this pull request? The sourceless node support makes sense to me, though.

@lgtm-com
Copy link

lgtm-com bot commented Sep 9, 2022

This pull request introduces 1 alert when merging 3010834 into f7e9cb5 - view on LGTM.com

new alerts:

  • 1 for Implicit operand conversion

Copy link
Collaborator Author

@arthurfiorette arthurfiorette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

anything else?

@arthurfiorette arthurfiorette changed the title feat: support for sourceless nodes and optional #/reference/ usage. feat: support for sourceless nodes. Sep 16, 2022
@arthurfiorette
Copy link
Collaborator Author

@domoritz, sorry for pinging, but this PR would help me so much :)

@domoritz
Copy link
Member

domoritz commented Sep 30, 2022

Thanks for the ping. It's in my queue to review but I have a lot on my plate right now. I'd recommend publishing your fork in your personal namespace to get unblocked.

Copy link
Member

@domoritz domoritz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Just one comment and then we can merge. Thanks for all the work on this!

src/NodeParser/TypeReferenceNodeParser.ts Outdated Show resolved Hide resolved
test/sourceless-nodes/index.test.ts Outdated Show resolved Hide resolved
test/sourceless-nodes/index.test.ts Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants