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

feat: Add fast-check based TypeScript type definition generation #379

Merged
merged 37 commits into from Feb 16, 2024

Conversation

marvinhagemeister
Copy link
Contributor

This PR adds the ability to generate TypeScript definition files (*.d.ts) when fast check is enabled.

@CLAassistant
Copy link

CLAassistant commented Feb 13, 2024

CLA assistant check
All committers have signed the CLA.

@marvinhagemeister marvinhagemeister marked this pull request as ready for review February 13, 2024 15:03
Copy link
Member

@dsherret dsherret left a comment

Choose a reason for hiding this comment

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

It's looking good, but we should spend some time handling unknown variants a bit before landing this so that we don't swallow errors and can be alerted better to problems (either by debug panics or surfacing diagnostics that dts generation failed).

src/fast_check/transform_dts.rs Show resolved Hide resolved
src/fast_check/transform_dts.rs Outdated Show resolved Hide resolved
src/fast_check/transform_dts.rs Outdated Show resolved Hide resolved
src/fast_check/transform_dts.rs Outdated Show resolved Hide resolved
src/fast_check/transform_dts.rs Outdated Show resolved Hide resolved
src/fast_check/transform_dts.rs Outdated Show resolved Hide resolved
src/fast_check/transform_dts.rs Show resolved Hide resolved
src/fast_check/transform_dts.rs Outdated Show resolved Hide resolved
src/fast_check/transform_dts.rs Outdated Show resolved Hide resolved
src/fast_check/transform_dts.rs Outdated Show resolved Hide resolved
@marvinhagemeister
Copy link
Contributor Author

@dsherret This is ready to be reviewed now. Made an integration into jsr as well.

Copy link
Member

@dsherret dsherret left a comment

Choose a reason for hiding this comment

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

Almost there!

fn gen_unique_name(&mut self) -> String {
self.id_counter += 1;
format!("_dts_{}", self.id_counter)
}
Copy link
Member

Choose a reason for hiding this comment

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

In the future we can make this better to actually avoid conflicts, but the chance of that happening are so slim (ex. do a pre-pass on the file and collect all the identifier names--including globals referenced in types and put them in a hashset, then here it would try something similar, but check for conflicts)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd prefer to fix that when it arises. It seems like super edge case and unlikely to happen in practice.

src/fast_check/transform_dts.rs Outdated Show resolved Hide resolved
src/fast_check/transform_dts.rs Show resolved Hide resolved
tests/specs/graph/JsrSpecifiers_FastCheck_TsModule.txt Outdated Show resolved Hide resolved
tests/specs/graph/JsrSpecifiers_FastCheck_Enums.txt Outdated Show resolved Hide resolved
@marvinhagemeister
Copy link
Contributor Author

@dsherret addressed the feedback and it's ready for another review.

}
}

Expr::TsAs(ts_as) => self.valid_enum_ts_type(*ts_as.type_ann),
Copy link
Member

Choose a reason for hiding this comment

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

I get an error for this in declaration files

In ambient enum declarations member initializer must be constant expression.ts(1066)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seems to work for a very small subset:

export enum Foo {
  A = 1 as number,
}

F = Foo1.A | Foo1.C,
G = Foo1.A + Foo1.C,
H,
I = 1 + NUM
Copy link
Member

Choose a reason for hiding this comment

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

It seems this is also not allowed in dts files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah wanted to add a couple cases where TS strips the type but still generates the files even with a type error.

Copy link
Member

Choose a reason for hiding this comment

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

We probably shouldn't follow TS here and instead just strip this with a warning diagnostic.

Copy link
Member

@dsherret dsherret left a comment

Choose a reason for hiding this comment

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

LGTM. Nice work!

@marvinhagemeister marvinhagemeister merged commit 34681b3 into main Feb 16, 2024
5 checks passed
@marvinhagemeister marvinhagemeister deleted the dts-generation branch February 16, 2024 15:36
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.

None yet

3 participants