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

Add type definition #31

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Add type definition #31

wants to merge 4 commits into from

Conversation

nadilas
Copy link

@nadilas nadilas commented Jun 11, 2022

Addresses #30

Copy link
Contributor

@ryan-blunden ryan-blunden left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @nadilas and I'll review next week.

@@ -1,5 +1,6 @@
const fs = require("fs");
const path = require("path");
const { jsonInputForTargetLanguage, InputData, quicktype } = require("quicktype-core");
Copy link
Author

Choose a reason for hiding this comment

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

Sorry, somehow I missed the imports when creating the patch

@nadilas
Copy link
Author

nadilas commented Jul 9, 2022

@ryan-blunden any way I can help?

@ryan-blunden
Copy link
Contributor

Sorry about the delay @nadilas. I’ll review this as soon as I can.

@nadilas
Copy link
Author

nadilas commented Nov 2, 2022

Hey @ryan-blunden, just a reminder, this one probably got lost.

@Piccirello
Copy link
Contributor

Thanks for the PR @nadilas. Adding types to this project is definitely a worthwhile endeavor.

This project has an explicit goal of having zero dependencies. That allows us to ensure that this project does not need to be patched as CVEs are found in external libraries. Unfortunately, this PR introduces a dependency on quicktype-core. Rather than rely on quicktype, I think it'd be preferable to convert this codebase to TS. The relatively small size of this repo should make that goal fairly manageable. I will note that we don't have any dedicated resources focused on this project at the moment, so I can't speak to when that change will occur. We'd definitely welcome a PR if that was something you're interested in.

@nadilas
Copy link
Author

nadilas commented Jun 23, 2023

Hi @Piccirello, thanks for the update. There might be a misunderstanding this PR adds types to the generated output, so its usage is type-safe, hence the quicktype dependency. and not to this codebase.

If there are no resources on your your end, I’m fine with publishing as a separate package if that’s ok with you.

@ryanleecode
Copy link

Hi @Piccirello, thanks for the update. There might be a misunderstanding this PR adds types to the generated output, so its usage is type-safe, hence the quicktype dependency. and not to this codebase.

If there are no resources on your your end, I’m fine with publishing as a separate package if that’s ok with you.

Was this ever published? This is a really important feature

@nadilas
Copy link
Author

nadilas commented Apr 22, 2024

@ryanleecode it wasn’t. How urgently do you need it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants