-
Notifications
You must be signed in to change notification settings - Fork 39
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: allow custom types with inner types #278
feat: allow custom types with inner types #278
Conversation
src/utils.ts
Outdated
@@ -255,6 +255,8 @@ export const typify = ( | |||
// we'll have to qualify it with the Node.js namespace. | |||
return 'NodeJS.ReadableStream'; | |||
} | |||
// Custom type | |||
if (innerTypes) return `${typeAsString}<${typify(innerTypes[0])}>`; |
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.
innerTypes
is an array, we should technically do this for any number of inner types not just the first one. E.g. Foo<T, R>
should be supported (I think the docs parser will read this)
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.
Confirmed. I tested with docs-parser and received multiple innerTypes (nice work btw!)
This will be blocked on electron/electron#43985 just FYI, but should be good to go if you have the CFA token merge at will. Otherwise ping me on Slack and I can merge / release it. |
electron/electron#43985 is merged so it shouldn't be blocking this PR anymore. @samuelmaddock @MarshallOfSound is this PR ready to merge? |
🎉 This PR is included in version 9.1.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
I'm considering making some of the IPC types support generics for electron/rfcs#8. While experimenting, I noticed we don't generate them for custom types.