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.

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

fix: support for generic functions #2159

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 [email protected]
# or 
yarn add [email protected]

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

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

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
@@ -93,6 +93,7 @@
"vega": "^5.30.0",
"vega-lite": "^5.21.0"
},
"packageManager": "[email protected]",
Copy link
Member

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

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

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
Copy link
Collaborator Author

@domoritz can I merge?

@domoritz
Copy link
Member

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

tks

@github-actions GitHub Actions
Copy link

🚀 PR was released in v2.4.0-next.7 🚀

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
prerelease
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@arthurfiorette @domoritz