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

feature: add JavaScript codegen #41

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

feature: add JavaScript codegen #41

wants to merge 18 commits into from

Conversation

croconut
Copy link

Summary

closes #39

Adds functionality to check the output file type to enable JS codegen. When .js or .mjs, will output JS ESM code, when .cjs, will output CommonJS code.

Both JS format types will output fully typed code using JSDocs.

Additionally, can override the file type with --output-type, which accepts TS, CJS, or ESM and defaults to TS. Will not change the extension used. This was added so you can generate a .js as CommonJS and a way to output CommonJS to stdout.

Why bother

I use JSDocs + the typebox check compiler to get fast runtime checks for external inputs.

@croconut
Copy link
Author

@xddq let me know if you want me to squash early, figured you could squash what you want on merge

Copy link
Owner

@xddq xddq left a comment

Choose a reason for hiding this comment

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

Hey @croconut ! Thanks for the work, looking good! 🚀

Did quickly skim over it on my phone and dropped mostly QoL(quality of life comments) and thoughts. Feel free to comment and perhaps also resolve. Also, forgive me if the comments sound (or are) stupid, just got a surgery and did not check the code on my pc at all. But wanted to at least provide initial feedback quickly.

Will perhaps be able to give this a deeper look in some weeks (perhaps days, but I think 2-3 weeks is realistic here).

Also a note in advance, feel free to bump this if I forgot to check it in 2-3 weeks.

README.md Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/cli/cli.ts Outdated Show resolved Hide resolved
@@ -72,6 +83,10 @@ export const getHelpText = {
--output-stdout
Does not generate an output file and prints the generated code to stdout
instead. Has precedence over -o/--output.

--output-type
Sets the output language, defaults to Typescript.
Copy link
Owner

Choose a reason for hiding this comment

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

Perhaps add something like determines the language and syntax of the generated code TS -> generated typescript code, cjs -> generates commonjs code with jsdoc, mjs ->generated modern (module..?) js code eith jsdoc. Has precedence over auotmated inference based on output filename.

And dont forget to also update the readme with the new text please!

Now that I write it, wouldn't commonjs be helpful for ts as well? I guess it is rare, but while we are on it..? Perhaps a CTS? Or we adapt the flag to "TS,commonjs" and "JS,esm" etc..?

Copy link
Author

Choose a reason for hiding this comment

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

this does not have precedence over automated inference as .js + output-type = TS would get changed to .js + output-type = ESM. Could alternatively declare that an invalid parameter set and error out i guess.

id rather not encourage anyone to use commonjs + typescript lol.

feel free to change the text on this PR if you want. Otherwise ill just copy paste what you write unless i think its inaccurate.

Copy link
Owner

Choose a reason for hiding this comment

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

Adapt the text as you want, would just like to resemble the new text with "js-esm", "js-cjs" etc. as mentioned in the comment

src/schema-to-typebox.ts Outdated Show resolved Hide resolved
tsconfig.json Outdated Show resolved Hide resolved
src/cli/cli.ts Outdated Show resolved Hide resolved
@croconut
Copy link
Author

croconut commented Mar 19, 2024

@xddq im so used to not being able to run formatters x.x mb. ran it / checked wfs on the fork now

Copy link
Owner

@xddq xddq left a comment

Choose a reason for hiding this comment

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

hey @croconut

sorry for the delay. Did another review and got one new remark. What is your plan going forward? When will you address the points that are left (and will you at all)? I am fine with releasing this as 1.8.0 once this is done.

best regards

@@ -101,11 +129,18 @@ export const collect = (schema: JSONSchema7Definition): Code => {
* Unused imports (e.g. if we don't need to create a TypeRegistry for OneOf
* types) are stripped in a postprocessing step.
*/
const createImportStatements = () => {
return [
const createImportStatements = (useCommonJs = false) => {
Copy link
Owner

Choose a reason for hiding this comment

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

Again, not to much of fan of default argument here. I would prefer making it required

@@ -32,8 +33,23 @@ import {

type Code = string;

export type SupportedFiletypes = "CJS" | "TS" | "ESM";
Copy link
Owner

Choose a reason for hiding this comment

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

Can we adapt this to something like this?

Suggested change
export type SupportedFiletypes = "CJS" | "TS" | "ESM";
export type OutputType = "js-cjs" | "js-esm" | "ts-esm";

@xddq xddq self-assigned this Apr 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feature request: generate to JavaScript with JSDoc types
2 participants