Skip to content

fix: support for generic functions #2159

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 12 commits into from
Feb 18, 2025
Merged

fix: support for generic functions #2159

merged 12 commits into from
Feb 18, 2025

Conversation

@arthurfiorette
Copy link
Collaborator

@arthurfiorette arthurfiorette commented Jan 23, 2025 •
edited by github-actions bot
Loading

Previously, functions like:

function fn<A>(a: A) {
  return { a }
}

const result = fn(1)
//             ^ throws

would throw even when not in use because it would parse the function without a reference to each parameter on their respective call, since A is generic but in the case of fn(1), A is number.

I added some extra security checks to avoid undefined errors being thrown without extra information about the actual node.

📦 Published PR as canary version: 2.3.1--canary.2159.32f3c45.0

✨ Test out this PR locally via:

npm install ts-json-schema-generator@2.3.1--canary.2159.32f3c45.0
# or 
yarn add ts-json-schema-generator@2.3.1--canary.2159.32f3c45.0

Version

Published prerelease version: v2.4.0-next.7

Changelog

🎉 This release contains work from new contributors! 🎉

Thanks for all your work!

❤️ Michael Matloka (@Twixes)

❤️ null@dcharbonnier

❤️ Werner Robitza (@slhck)

❤️ Bence Balogh (@baloghbence0915)

🚀 Enhancement

🐛 Bug Fix

⚠️ Pushed to next

🔩 Dependency Updates

Authors: 7

@arthurfiorette arthurfiorette self-assigned this Jan 23, 2025
@arthurfiorette
Copy link
Collaborator Author

arthurfiorette commented Jan 27, 2025

Probably continuation of #1978

domoritz
domoritz requested changes Jan 27, 2025
View reviewed changes
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.

Tests are failing but overall looks reasonable.

@arthurfiorette
Copy link
Collaborator Author

arthurfiorette commented Feb 17, 2025

Hey @domoritz, sorry for the delay, should be working now.

Instead of crashing with Cannot access property X of undefined when the parser fails to generate a type, I added this second check to ensure we either get a type or it throws useful information to create a bug report for us to fix it:

ts-json-schema-generator/src/ChainNodeParser.ts

Lines 46 to 48 in 6f776af

if (!type) {
throw new UnknownNodeError(node);
}

Where this incapacity is expected, we should be adding a || new UnknownType(true) so the whole generation doesn't fail and the user at least ends up with a partial generation because most of the time these failures aren't in an object/field that will end up in the JSON schema.

If somehow a UnknownType(true) ends up in the generated schema, the json should look like this:

{
    type: "object",
    properties: {
        fieldThatFailed: { description: "Failed to correctly generate type" },
    },
}

So we still give a hint for this unexpected unknown in the final schema.

domoritz
domoritz approved these changes Feb 17, 2025
View reviewed changes
package.json
"vega": "^5.30.0",
"vega-lite": "^5.21.0"
},
"packageManager": "npm@10.8.2",
Copy link
Member

@domoritz domoritz Feb 17, 2025

Choose a reason for hiding this comment

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

Why is this needed?

Copy link
Collaborator Author

@arthurfiorette arthurfiorette Feb 17, 2025

Choose a reason for hiding this comment

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

corepack is a good standard when working with a lot of different package managers on a lot of different projects.

This field is the standard field that a lot of tools (including corepack) will use to determine the package manager.

I can remove it if you want

Copy link
Member

@domoritz domoritz Feb 18, 2025

Choose a reason for hiding this comment

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

I just don't feel like we specifically need that version of npm. Yeah, let's remove it since we don't consistently use it in this org.

arthurfiorette and others added 2 commits February 17, 2025 11:39
@arthurfiorette @domoritz
Update src/NodeParser/TypeReferenceNodeParser.ts
c1482c7 
Co-authored-by: Dominik Moritz <domoritz@gmail.com>
@arthurfiorette
fix better msg
32f3c45
@arthurfiorette
Copy link
Collaborator Author

arthurfiorette commented Feb 18, 2025

@domoritz can I merge?

@domoritz
Copy link
Member

domoritz commented Feb 18, 2025

Yes, go ahead.

@arthurfiorette arthurfiorette merged commit c8ea3ed into next Feb 18, 2025
3 checks passed
@arthurfiorette arthurfiorette deleted the arthur/fix-generic-fn branch February 18, 2025 03:29
@arthurfiorette
Copy link
Collaborator Author

arthurfiorette commented Feb 18, 2025

tks

@github-actions
Copy link

github-actions bot commented Apr 6, 2025

🚀 PR was released in v2.4.0 🚀

@github-actions github-actions bot added released This issue/pull request has been released. and removed prerelease labels Apr 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@domoritz domoritz domoritz approved these changes

Assignees

@arthurfiorette arthurfiorette

Labels

released This issue/pull request has been released.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@arthurfiorette @domoritz