Conversation
|
@xddq let me know if you want me to squash early, figured you could squash what you want on merge |
xddq
left a comment
xddq
left a comment
There was a problem hiding this comment.
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.
| instead. Has precedence over -o/--output. | ||
|
|
||
| --output-type | ||
| Sets the output language, defaults to Typescript. |
There was a problem hiding this comment.
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..?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Co-authored-by: Pierre Dahmani <hi@pierre-dev.com>
|
@xddq im so used to not being able to run formatters x.x mb. ran it / checked wfs on the fork now |
There was a problem hiding this comment.
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
| */ | ||
| const createImportStatements = () => { | ||
| return [ | ||
| const createImportStatements = (useCommonJs = false) => { |
There was a problem hiding this comment.
Again, not to much of fan of default argument here. I would prefer making it required
|
|
||
| type Code = string; | ||
|
|
||
| export type SupportedFiletypes = "CJS" | "TS" | "ESM"; |
There was a problem hiding this comment.
Can we adapt this to something like this?
| export type SupportedFiletypes = "CJS" | "TS" | "ESM"; | |
| export type OutputType = "js-cjs" | "js-esm" | "ts-esm"; |
|
This would be nice to have! |
3 participants
Summary
closes #39
Adds functionality to check the output file type to enable JS codegen. When
.jsor.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
.jsas 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.