Aleph-Alpha / ts-rs

Generate TypeScript bindings from Rust types
MIT License
1.15k stars 115 forks source link

Proof of concept, argument to `export_to` other than `&'static str` #347

Open NimonSour opened 3 months ago

NimonSour commented 3 months ago

Hi there, and thank you for this crate!

In addition to a 'static str as the argument to export_to attribute we could use the following types :

- name of a function (`Fn(&'static str) -> Option<&'static str>`) that will override the standard `TS::output_path` method logic. 

example:
```rust

// ts_name is provided by TS macro (name of the ts type)

pub fn get_path_fn(ts_name: &'static str) -> Option<&'static str> {
    match ts_name {
        "ObjA" => Some("../ts-fn-path/ObjA.ts"),
        "ObjB" => Some("../ts-fn-path/B/ObjB.ts"),
        _ => None, 
    }
}

the argument is an identifier or a path to our function

#[ts(export_to = get_path_fn)]

#[ts(export_to = crate::get_path_fn)]

TS::output_path method generated for the ( ts name ObjB):

    fn output_path() -> Option<&'static std::path::Path> {
        Some(std::path::Path::new(crate::get_path_fn("ObjB")?))
    }
static MY_STATIC_PATH: &'static str = "../ts-static-path/";
const MY_CONST_PATH: &'static str   = "../ts-const-path/";

again as identifier or path

#[ts(export_to=crate::MY_STATIC_PATH)]
#[ts(export_to=MY_CONST_PATH)]

TS::output_path method generated for the type:

    fn output_path() -> Option<&'static std::path::Path> {
        static PATH: std::sync::OnceLock<String> = std::sync::OnceLock::new();
        let path = PATH
            .get_or_init(|| {
                if crate::MY_CONST_PATH.ends_with('/') {
                    format!("{}{}.ts", crate ::MY_CONST_PATH, "ObjA")
                } else {
                    format!("{}", crate ::MY_CONST_PATH)
                }
            });
        Some(std::path::Path::new(path))
    }

All options are reactive, modifying the source of argument value will make TS export to the new path without need to recompile.

Thinking about the most efficient or correct way to handle paths can be overwhelming, as there's no universally right way to do it! My proposition is to allow users to manage paths using methods already established by the programming community: constants, statics, and environment variables. Unfortunately, due to Rust's limitations for Derive macros and the 'test' runtime, this is currently restricted to those defined in .cargo/config.toml.

NyxCode commented 2 months ago

Hey, thanks for opening this PR!

Is there a real use-case behind this feature, or is this more of a "it'd be really cool if this worked" kind of thing?
We do already allow altering the export directory by setting the TS_RS_EXPORT_DIR variable. The idea here is that the exported bindings always have the same directory structure, and we only allow users to specify where the bindings will end up.

I do think there are some advantages of keeping the directory structure static. For more exotic usecases (like custom build scripts), there's TS_RS_EXPORT_DIR.

NimonSour commented 2 months ago

Hi NyxCode!

It's not that providing a real-life example is difficult, but any example could be challenged and redirected to a workflow where the argument is a &'static str. What is the point for such restriction in a development tool ?

One general downside of using a &'static str as a path is that in case of change it forces the user to manually change the export_to argument and recompile every instance of the same path value. This limits flexibility by ignoring the concept of a variable, which is fundamental in programming.

ts-rs is a valuable development tool, and if it requires a path to write to, IMO it should be flexible enough to accommodate the various ways developers handle paths.

NimonSour commented 2 months ago

We do already allow altering the export directory by setting the TS_RS_EXPORT_DIR variable. The idea here is that the exported bindings always have the same directory structure, and we only allow users to specify where the bindings will end up.

Looks like we're talking about two different aspects here. I understand your concerns over TS_RS_EXPORT_DIR crate convention. But one can still define a path (as &static str) relative to TS_RS_EXPORT_DIR that’s outside of it. The risk of the writer deleting custom code to make room for a TS-generated type is the same regardless of whether the path was provided as a const or &static str.

Honestly, I didn’t even consider any side effects while implementing this PR; my approach was "Just trust the trait and make sure TS::output_path returns what the user provided as a path value".

The only purpose of this PR is to challenge the type of input for the export_to argument.

Especially after the latest releases, users (myself included) can now write multiple types to the same file. So why should I have to provide the same path as a &'static str for, say, three different types if I want them written in the same file? I this case changing my mind about TS types location, I have two options:

a) Copy and paste the new path to each of my three objects, recompile, and run test again. b) Physically move the types file to the desired location and sort out the import paths if necessary.

However, if the path is defined as lets say an environment variable, all I need to do is:

1) Delete the old file with the ts types. 2) Change the environment variable's value and run test.

So yes, the arguments might look cooler, but there's more than that. Typically, for a group of objects, we define shared variable instead of hard-coding values. The compiler only checks these variables, and in the case of a &'static str, it will silently accept one with a typo, increasing the risk of bugs.

TS_RS_EXPORT_DIR convention is something that I simply didn't take in account, possibly it may be confusing for the user I don't know, the only point of this PR is the type of input for export_to.

gustavo-shigueo commented 2 months ago

This looks really interesting! 2 questions though:

NimonSour commented 2 months ago

Hi @gustavo-shigueo !

One more thing to consider is documenting the type options. The ts-rs documentation is already quite dense with terms, and adding more might make it harder for users to remember any.

Plus the fact that macros are really bad when it comes to documenting.

I propose to introduce an easy to remeber notation like this #[ts(export_to= ? )] Which will resolve compile-time error that includes comprehensive information about the TS_RS_EXPORT_DIR convention and path argument types.

Bassically a quick reminder.

Compared to crate documentation this could be more efficient as the information is provided at the right place and time.

gustavo-shigueo commented 2 months ago

I propose to introduce an easy to remeber notation like this #[ts(export_to= ? )] Which will resolve compile-time error that includes comprehensive information about the TS_RS_EXPORT_DIR convention and path argument types.

That may not be necessary, the error message you've written already seems to serve that purpose quite well, so if we make sure to display it for any syntax error in export_to, that should lead the user in the right direction

NyxCode commented 2 months ago

Before too much effort is put into this, i think we should answer the question if we really want this feature.
I am yet to be convinced of the benefit, and I dislike the complexity this adds.

gustavo-shigueo commented 2 months ago

I see, I do kinda like the idea of allowing consts and statics on top of &'static str. I don't really like the function path or even the environment variables though.

As for the complexity, it would go way down if only static/const were added, but I'm also not sure it'd be worth it

NimonSour commented 2 months ago

Sorry for the delay, I can't actively includ myself in conversation right now ( will be back in couple of days).

Think of other proposition, I'll write later some arguments in favour of keeping all variants of Custom Path and and probably the notation I've proposed, we could just include it as a separate feature (ex."typed path") not that I've donne it but I think it should be relatively easy to implement as Custom Path is well modulate already, very litle friction with the rest of the crate. Think of pros and cons in this direction, befor I write the arguments that will conclude to this option.

NimonSour commented 2 months ago

This PR was initiated to demonstrate the possible use of different types for the export_to attribute argument. The current version of the crate accepts only &'static str, which may be sufficient for most users, especially for small projects. However, larger projects are likely to encounter limitations.

Naming types, modules, and variables is always important, and the right names often emerge as the project evolves. Therefore, having a flexible approach to renaming both the types of arguments and the values of those arguments is a common and necessary practice for programmers.

Since I didn't have a clear initial objective for this PR, I think it would be helpful to summarize my proposition in one clear statement.

Allowed Types:

1) static and const variables. 2) a function pointer to override the TS::output_path method. 3) environment variables declared in .cargo/config.toml. No support for normal environment variables (std::env::var) due to limitations in the Derive macro and test runtime.

In my opinion, using a typed path is safer for various reasons. Limiting the crate to the use of &'static str to avoid potential bugs introduces other issues, particularly related to the inert nature of &'static str in the compiler.

The current PR aims to assist in two major ways:

1) Renaming a type will trigger error messages in the text editor for every module where the type was used, reducing the effort needed to resolve issues — a very common workflow.

2) All variants of CustomPath are directly accessed within TS::output_path, meaning any value changes will be instantly accessible by TS::output_path without needing to recompile.

The crate's responsibility is limited to two general cases:

1) Informing the user about the TS_RS_EXPORT_DIR crate convention and how TS::output_path modifies the initial path. This may include details about potential issues that could arise when working outside of the convention.

Note: This point is base for ? notation for export_to propossed earlier.

2) Preventing any misinterpretation of type variants of CustomPath

Additionally, I believe that restricting the number of variants, whatever they may be, lacks sufficient justification. Since there isn't a universally efficient approach, any type restriction is often unnecessary. The type of the path itself isn't likely to cause bugs; rather, it's the type value that could lead to issues, which is entirely the user's responsibility.

Consider either accepting Custom Path for ts-rs or including it as a separate feature that allows typed path arguments for the export_to attribute.

I've made my point, from here, it's up to you to decide what is acceptable so we can move forward with more precise code, some tests, and ensure that the PR aligns with project requirements.

NimonSour commented 2 months ago

There was a bug, it wasn't working for the environment variables, after refactoring. 😅 Sorry for the confusion if any!