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

Improvements to Generating typescript types #2186

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

AmmarAbouZor
Copy link
Member

This PR closes #2181

It provides the following:

  • Sets the path for the generated files directly to platform/types/bindings to avoid the manual copying and having two sets for generated files at the same time.
  • Add note on all types that are manually adjusted after generation to rule out any accidentally changes in the future.
  • Apply formatting on all generated files to ensure that all of them have the same formatting enforcing further changes to apply the formatting as well. (This can be automated in separate PR)

* Define environment variable to set the output directory for the
  generated typescript files.
* Env variable is defined inside a cargo configuration file in `stypes`
  crate.
* Update the comments in code and scripts accordingly.
Add notes on code on the manually adjusted code in typescript files to
avoid changing them accidentally in the future.
* Those files aren't used anymore and can be removed after moving their
  comments to platform file in the previous commands.
* This formatting shouldn't have changed anything in the code.
* This is needed to ensure we don't get any changes in any further
  regeneration of the types after enforcing the formatting
@DmitryAstafyev
Copy link
Collaborator

DmitryAstafyev commented Jan 31, 2025

@AmmarAbouZor using environment variables is not the most "transparent" approach. We want developers to understand what’s happening and how the protocol build process works, rather than relying on hidden "magic." Storing configuration in hidden files like ./.cargo/config.toml makes the solution even less obvious for external developers.

Improving the build system is a great goal, but I suggest sticking to a more conventional approach. If we want to make copying "automatic," let’s simply add a build.sh script that handles copying to the correct location and cleaning up temporary files. A straightforward script will make it easier for developers to understand what’s happening and how the process works.

We don't need in this scope (stypes) to support multiple platform, and having shell scrip totally fine.

@AmmarAbouZor
Copy link
Member Author

Actually, I've followed the recommended way to set the export path of the generated types from ts-rs Documentation.
Besides, that I've written comments in multiple places in code to explaining how it's configured.

Personally, I don't find it more convenient to add extra step by manually copying the files after generating them that lives outside of the rust-code base in an external bash script. I think that having the solution inside rust code with the documentation as recommended by the crate developers is more convenient.

@DmitryAstafyev
Copy link
Collaborator

I understand that you followed the recommended approach from the ts-rs documentation. However, "recommended" doesn’t always mean "best for every project."

The issue here is not whether the solution is technically possible, but how easy it is to discover and understand. Scattering comments in multiple places and relying on a hidden .cargo/config.toml file makes the process less transparent for new developers. Someone cloning the repo won’t immediately know that they need to tweak a hidden config file before running the build. And looking for explanation in docs of ts-rs. Not everyone is deeply familiar with Cargo internals, and a hidden .cargo folder can be confusing.

A separate build.sh script would provide a single, clear entry point. No need to dig through documentation or track down scattered comments—just run one script, and everything works. This keeps things explicit and maintainable for the team in the long run. Any necessary comments, by the way, can be included directly in the bash script.

Would you be open to discussing an approach that balances automation with clarity?

@DmitryAstafyev
Copy link
Collaborator

In general, including hidden folders like .cargo in a project doesn’t seem like the best approach. If I’m not mistaken, this directory is typically meant for local configurations, which could be confusing for users.

The fact that ts-rs recommends this method is not necessarily a decisive argument. After all, they could change their API or configuration approach at any time, and we would have to adjust on our side. That would create an unnecessary dependency—especially considering that the alternative is just a simple script.

Also, there are quite a few open questions about ts-rs itself. It’s not necessarily the best solution—just the only viable one for now.

@AmmarAbouZor
Copy link
Member Author

Sure, the approach from ts-rs team isn't optimal by any mean. It's just as none-optimal as having a script to manually move items after generation.
I understood that .cargo/config is and for project specific configuration and it's not a per-user link.
I think it's similar to how .github directory is a dot directory that lives with project repository.

Anyway, the two approaches aren't optimal in my opinion, and I can change it to manually copying if you find it better

@DmitryAstafyev
Copy link
Collaborator

I would say like it: if I would have my own private project, I would select your suggested way (using .cargo/config.toml); if I'm choosing for public repo - I would follow transparency for other users, even I could not like something ... that's life ;)

@AmmarAbouZor
Copy link
Member Author

It would be actually interesting to get more opinions about this, because I still can't imagine a scenario where copying files manually in a separate script is the more convenient option.

@marcmo @kruss Could you please give us some insights here.

tldr:*
The suggested way from ts-rs crate to control the path of the generated typescript files is to set an environment variable inside cargo configuration file located in stypes/.cargo/config.toml, which is implemented here with comments in the code.
@DmitryAstafyev thinks that this is confusing for developer since the configuration file is somehow hidden for developers and it would be less confusing to keep the files generated in the default path and extend the bash script for generating the files with other commands to move those generated files to the wanted destination after generating them.

@DmitryAstafyev Please feel free to extend this short description if I have missed something here

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.

Improvement for generating TS types
2 participants