-
Notifications
You must be signed in to change notification settings - Fork 42
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
base: master
Are you sure you want to change the base?
Conversation
* 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
@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 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 We don't need in this scope (stypes) to support multiple platform, and having shell scrip totally fine. |
Actually, I've followed the recommended way to set the export path of the generated types from 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. |
I understand that you followed the recommended approach from the 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 A separate Would you be open to discussing an approach that balances automation with clarity? |
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. |
Sure, the approach from Anyway, the two approaches aren't optimal in my opinion, and I can change it to manually copying if you find it better |
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 ;) |
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:* @DmitryAstafyev Please feel free to extend this short description if I have missed something here |
I’m very grateful for this initiative. The desire to make the solution cleaner and better is always welcome. However, we have already discussed this topic in detail, a decision has been made, and our priority is transparency, clarity for the majority of users, and minimizing dependencies on third-party APIs. Besides, we already have test.sh to run tests, and we are already using shell scripts to manage this sub-project. Also, as mentioned last Tuesday, I specifically asked not to spend time on this but to focus on high-priority tasks. I don’t believe it’s productive to continue this discussion at this point, nor to take up our colleagues’ time on a secondary matter. Therefore, I’m closing this PR. We can revisit it later, once high-priority tasks are completed - and only if we encounter any actual issues that need to be addressed. At the moment, I don’t see any such issues. Please, let’s focus on the priority work. |
I think we can keep this PR open, but we can mark it as a draft until we get time to take a decision about it so we don't forget it since this PR already provide a solution in it |
This PR closes #2181
It provides the following:
platform/types/bindings
to avoid the manual copying and having two sets for generated files at the same time.