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

Include type declarations with @dataform/core #1647

Merged
merged 1 commit into from
Jan 15, 2024
Merged

Include type declarations with @dataform/core #1647

merged 1 commit into from
Jan 15, 2024

Conversation

bmagyarkuti
Copy link
Contributor

A suggested, trivial resolution for #1646.

I understand the typings were removed in #1552 to minimize the installed footprint of the @dataform/core package.

While re-adding the declarations file increases the bundle size, the size reductions introduced by #1552 would remain very similar. Before the introduction of minification, the total node_module size was 9.4MB. After the minifaction PR, it was ~432KB, and with the d.ts file re-added it would increase back to ~675KB. Instead of the original ~20x reduction, we just get a ~16x reduction. This increase can not impact compile performance, as the types file is not used during run time, so that performance gain remains.

This is just a suggestion. As I mentioned in the issue, my goal is to get type declarations for the @dataform/core package.

I'm very happy to assist with any solution that achieves this, including publishing type declarations in a separate package, possibly to the Definitely Typed repository.

I'd like to nominate @lewish as reviewer, since he introduced #1552 .

@BenBirt BenBirt requested a review from lewish January 15, 2024 09:51
@BenBirt
Copy link
Collaborator

BenBirt commented Jan 15, 2024

Just some added context: it turns out that #1552 did not actually speed up compilation time (that was a red herring). But we originally did it to speed up package installation time (and that certainly did work).

My 2c: clearly some users need typings, so we have two options: bring them back, or export a separate package including types. I think I'd rather bring back typings into the same package - it's simple, easy, and is pretty low-cost. We can always change our minds later!

@lewish
Copy link
Collaborator

lewish commented Jan 15, 2024

I'm OK with this! The files will rarely be loaded for most users, and it's not that big an increase.

It is slightly weird that the types are generated via rollup and the code via webpack, but I don't obviously see why that would cause any issues.

@lewish lewish merged commit c6feb01 into dataform-co:main Jan 15, 2024
2 checks passed
@bmagyarkuti
Copy link
Contributor Author

Thanks for the quick response! I'm so glad we get to continue using Dataform with TS (❁´◡`❁).

moker-spaghetti pushed a commit to moker-spaghetti/dataform that referenced this pull request May 26, 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.

3 participants