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 request: Deduplicate helpers and helper types across multiple generated files #1109

Open
absidue opened this issue Sep 20, 2024 · 3 comments

Comments

@absidue
Copy link

absidue commented Sep 20, 2024

Currently when multiple proto files are passed in, the helper functions such as isSet, longToNumber, bytesFromBase64 and base64FromBytes and the helper types MessageFns, Builtin, DeepPartial and KeysOfUnion are written into every single file. To reduce that duplication it would be great if those helpers and helper types were written into a separate file once and then imported where they are needed, that way they would only appear once in the output. Similar to how TypeScript's importHelpers option works, but importing from generated file instead of an external dependency.

If that extra file and imports are undesirable for Node.js use, it could be hidden behind the --ts_proto_opt=env=browser option, as people targetting web browsers usually want smaller code sizes and use bundlers which are able to handle the extra imports. To retain compatiblity with the existing output, the individual files could re-export the types.

Example:

  • generated
    • __ts_proto_helpers__.ts
    • example_request.ts -> imports from __ts_proto_helpers__.ts
    • example_response.ts -> imports from __ts_proto_helpers__.ts
@stephenh
Copy link
Owner

Hi @absidue ; I agree this would be a good improvement.

You mention making it conditional based on env=browser; not against that, but honestly I wouldn't mind just trying it, as the default output with no conditionals/no flags, and see if that works for everyone.

It would be amazing if you could work on a PR that does this; lmk if you have any questions/want any pointers about where to start. Thanks!

@absidue
Copy link
Author

absidue commented Oct 5, 2024

I have not looked into the codebase much yet, but here is my proposal on how something like this could potentially be implemented. Please let me know what you think about it and if you have answers to some of the questions below.

I would suggest generating and outputting the helper file right at the end, that way we only need to do it once. To do that we would need some way of keeping state across the different files that are processed so that we can keep a tally of what we actually need to output into the helpers file. Without knowing the codebase, in my head it makes the most sense to do that in the place that currently writes the helpers to the file, so instead of writing the helper or the type, it would record that it needs it in the "global" state and then generate an import instead.

Potential difficulties and open questions:

  1. To keep the code simpler, I would suggest outputting the separate helper file all the time, even if the user only passes in a single proto file, that will also make the behaviour more predictable from the perspective of the users.
  2. We would need to make sure that the helper file doesn't conflict with any of the other generated files, this could either be done by preventing users from naming their files the same as the helper file or by ensuring we generate a unique name if there is a conflict, the latter would make it more difficult for the people using the generated files, as the file name would be unpredictable.
  3. As the output can be nested into various different folders we need to be able to generate correct relative imports even if the proto file output is nested in various directories.
  4. Should we re-export the imported helpers and types in the generated proto files to remain backwards compatible?

@stephenh
Copy link
Owner

stephenh commented Oct 6, 2024

we can keep a tally of what we actually need to output into the helpers file.

Currently we use the Conditional Output feature of ts-poet to do track "what helper functions are/are not actually used in the generated output":

https://github.com/stephenh/ts-poet?tab=readme-ov-file#conditional-output

https://github.com/stephenh/ts-proto/blob/main/src/main.ts#L521

I don't remember if ts-poet's conditional output tracking works across files 🤔. Originally ts-poet was very file-oriented, and still is, but it's also got a saveFiles method that is more multi-file aware, so maybe that would be useful/helpful, not sure. Or maybe it already works 🤷 .

  1. ...outputting the separate helper file all the time...

Yep, agreed!

  1. ...avoid conflicting names...

Agreed; I think a file name like ts-proto-fns.ts would surely be unique.

  1. ...relative imports...

ts-poet has some helpers to do this sort of thing.

  1. ...re-export the helpers...

With this new approach, maybe we should default the "barrel file" / index.ts config flag to always on, i.e. deprecate the flag, and always enable the behavior, as then we could export the utility files from the index.ts file, and not worry about having to re-export them from each *.proto file's *.ts file.

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

No branches or pull requests

2 participants