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

proof-of-concept of type overrides using the CLI #334

Draft
wants to merge 8 commits into
base: cli
Choose a base branch
from

Conversation

NyxCode
Copy link
Collaborator

@NyxCode NyxCode commented Jun 21, 2024

This PR implements a POC of how we could implement type overrides using the CLI.

Comment on lines +1 to +5
[overrides]
u64 = "number"
i64 = "number"
u128 = "number"
i128 = "number"
Copy link
Collaborator Author

@NyxCode NyxCode Jun 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. A user creates a ts-rs.toml config file in their project

Comment on lines +37 to +40
for (rust, ts) in &cfg.overrides {
let env = format!("TS_RS_INTERNAL_OVERRIDE_{rust}");
cargo_invocation.env(env, ts);
}
Copy link
Collaborator Author

@NyxCode NyxCode Jun 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. The CLI reads the config file, and sets an environment variable for each override

Comment on lines +632 to +636
fn name() -> String {
$crate::get_override(stringify!($ty))
.unwrap_or($l)
.to_owned()
}
Copy link
Collaborator Author

@NyxCode NyxCode Jun 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. In ts-rs, we read the environment variable (cached!), and use it instead

@NyxCode
Copy link
Collaborator Author

NyxCode commented Jun 21, 2024

I am in no way sure that this is the right way to implement this. Besides the implementation (are environment variables really the right choice here?), these overrides would apply to the whole dependency chain. That might be the right behavior, but I'm not sure.

}

fn load_from_file() -> Result<Self> {
// TODO: from where do we actually load the config?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there should be a CLI flag like --config <PATH> and ./ts-rs.toml or maybe ./ts-rs/config.toml should be a default location if the flag isn't provided

@gustavo-shigueo gustavo-shigueo mentioned this pull request Jun 26, 2024
12 tasks
@gustavo-shigueo
Copy link
Collaborator

I am in no way sure that this is the right way to implement this. [...] (are environment variables really the right choice here?)

I like this implementation, and I'm not sure there even is another option to get cargo test to read the overrides without environment variables

Besides the implementation [...], these overrides would apply to the whole dependency chain. That might be the right behavior, but I'm not sure.

I think this is the right behavior. If, for example, the user wants u64 to be treated as number, they probably want it like that all the way up the chain

@gustavo-shigueo gustavo-shigueo added the CLI This issue/PR is related to or requires the implementation of the CLI tool label Jun 28, 2024
@gustavo-shigueo
Copy link
Collaborator

Hey @NyxCode! Is there anything else you'd like to do before merging this into the cli branch?

@gustavo-shigueo
Copy link
Collaborator

gustavo-shigueo commented Feb 21, 2025

Hey @NyxCode, one question I have about this: why do we need one env variable for each type override? Couldn't a single env variable work, like TS_RS_INTERNAL_OVERRIDE=String,string;i64,number (ofc the separators I used here are arbitrary and can be replaced by anything else, as long as neither TS or Rust consider it a valid character inside identifiers)

Alternatively, we could make ts-rs itself read the overrides from the file and have an env variable determine the path it reads from, though that would require adding a toml parser to ts-rs as well as the CLI, which is not ideal

@NyxCode
Copy link
Collaborator Author

NyxCode commented Feb 24, 2025

Yeah, I just did what seemed the most straight-forward here. Both reading them from a file (& caching it in a global?) and merging them into one environment variable are certainly possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLI This issue/PR is related to or requires the implementation of the CLI tool
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants