Embed types into the source code

[JonathanDagan]

Embed types into the source code

[danpaz]

Branch is created: https://github.com/danpaz/bodybuilder/tree/ts-migration

[johannes-scharlach]

@johnnyraphic Take a look at https://github.com/johannes-scharlach/elasticbuilder

2 years ago I did an almost complete re-write of bodybuilder in TS while copying most of the tests from this project. And since bodybuilder hasn't really changed since then this should still be up to date.

You can think of that project as a major update to bodybuilder with a breaking API change (and without any testing in production so far).
What's missing to merge it back into this project is backwards compatibility to the (very versatile) bodybuilder API. I believe that that's actually possible, so if you want to make bodybuilder work with TS, that's probably the easiest path.

[danpaz]

FWIW I'd be fully supportive of this, it's been a while since the TS type definitions were added and there for sure is room to improvement.

Is it possible to implement this in pieces instead of a complete rewrite? e.g. with allowJs: true in tsconfig

[johannes-scharlach]

Back when I did that rewrite, I saw no good way to get started on a gradual approach. I don't recall all the details why and now when you compare my rewrite with bodybuilder, you see that the file structure is very similar.

It might be possible to compare the two repos and figure out some ways to do this gradually.

I have in the end completely changed the way data is represented internally in the builders without affecting the external API. There because the internal state within bodybuilder isn't covered with any JSDoc types.

[JonathanDagan]

@danpaz To be honest you could do something like you suggested with allowing js to do a partial migration but tbh i don't see why you would like to do that since the code base isn't that big imo to go for that approach since it would be more of a hindrance than helpful imo.

Is there any reason I'm missing here with the partial migration to TS?

[danpaz]

The main reason would be to make the migration more manageable and we can transition to full TS in multiple PRs instead of just one big one. And this makes it less risky since we can't easily verify everything was ported correctly. But you are right it's not that much code so it may not be worth the trouble.

…

On Sat, Mar 11, 2023, 9:44 AM JonathanDagan ***@***.***> wrote: @danpaz <https://github.com/danpaz> To be honest you could do something like you suggested with allowing js to do a partial migration but tbh i don't see why you would like to do that since the code base isn't that big imo to go for that approach since it would be more of a hindrance than helpful imo. Is there any reason I'm missing here with the partial migration to TS? — Reply to this email directly, view it on GitHub <#313 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABLHENNXKCQ3FDYDCEWX6ZDW3SFVVANCNFSM6AAAAAAVVDFKAI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[JonathanDagan]

so I would recommend that we create a ts migration branch or something in the repo and than we can create multiple PRs just as you said and that would be more manageable to that branch and once that branch is ready we will merge it to the master branch and create a new release that supports es import properly

[danpaz]

👍 sounds good! Thanks for kicking this off

[JonathanDagan]

Sure thing :)

I'll wait for you to create the branch and you can tag me or something and ill start working on that.

I would recommend that if you have any specification as to how you would like stuff to be typed or as to how stuff would look out, to write it out before people would start working on that to try and make this as efficient as possible.