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, argument to export_to other than &'static str #347

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

NimonSour
Copy link

@NimonSour NimonSour commented Aug 16, 2024

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 :

  • an environment variable defined in .cargo/config.toml
#[ts(export_to = env("CONFIG_ENV_VAR_PATH"))]

TS::output_path method generated for the type:

    fn output_path() -> Option<&'static std::path::Path> {
        let path = if std::env!("CONFIG_ENV_VAR_PATH").ends_with('/') {
            std::concat!(std::env!("CONFIG_ENV_VAR_PATH"), "ObjB", ".ts")
        } else {
            std::env!("CONFIG_ENV_VAR_PATH")
        };
        Some(std::path::Path::new(path))
    }
  • name of a function (Fn(&'static str) -> Option<&'static str>) that will override the standard TS::output_path method logic.

example:

// 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")?))
    }
  • For users of Rust 1.7.0 and higher const and static names
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.

  • I have followed the steps listed in the Contributing guide.
  • If necessary, I have added documentation related to the changes made.
  • I have added or updated the tests related to the changes made.

@NyxCode
Copy link
Collaborator

NyxCode commented Aug 24, 2024

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
Copy link
Author

NimonSour commented Aug 24, 2024

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
Copy link
Author

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
Copy link
Collaborator

This looks really interesting! 2 questions though:

  • You've labeled this PR as a proof of concept. Why is that? Are there any changes you think need to be made?
  • Can you add some tests? That'll help make sure the feature doesn't break in the future

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

Note: This option is available for Rust 1.7.0 and higher!
Copy link
Collaborator

@gustavo-shigueo gustavo-shigueo Aug 27, 2024

Choose a reason for hiding this comment

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

This feels like a typo, did you really mean 1.7.0? The MSRV for ts-rs is 1.63.0, so this note is not needed if you really did mean 1.7.0

Copy link
Author

Choose a reason for hiding this comment

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

It's not a typo std::sync::OnceLock is stabilised in Rust 1.70.0, probably available 1.63. nightly only.

The idiomatic way is to check the Rust version, but #[cfg(version(..))]) isn't stabilised as well.

The worst-case scenario is a compile-time error, informing the user that std::sync::OnceLock is only available in nightly.

Additionally, since ts-rs currently depends on lazy_static, there's a good chance that this dependency will be dropped in favour of the standard library types increasing the MSRV and making this issue less relevant over time.

not sure what to do with it it's up to you really.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So you meant 1.70.0 right?

Copy link
Author

Choose a reason for hiding this comment

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

of course 1.70 )))

Copy link
Collaborator

Choose a reason for hiding this comment

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

And the code says 1.7... you're missing a zero

Copy link
Author

Choose a reason for hiding this comment

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

That's right, it is a typo )))

Copy link
Collaborator

Choose a reason for hiding this comment

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

Alright, in that case, we can just change the MSRV to 1.70.0 and we don't have to worry about it

Copy link
Collaborator

Choose a reason for hiding this comment

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

Formatting feels weird, can you run cargo +nightly fmt?

Comment on lines 99 to 124
let msg =
"expected arguments for 'export_to':

1) string literal
#[ts(export_to = \"my/path\")]

2) static or constant variable name

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

Note: This option is available for Rust 1.7.0 and higher!

3) function name of a `Fn(&'static str) -> Option<&'static str>`

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

Note: This option overrides the original `TS::output_path` logic`!

4) environment variable name

#[ts(export_to = env(\"MY_ENV_VAR_PATH\"))]

Note: This option is for environment variables defined in the '.cargo/config.toml' file only, accessible through the `env!` macro!
";
Copy link
Collaborator

Choose a reason for hiding this comment

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

This message should be a module level const to avoid having a massive string literal inside the function. This makes the function more difficult to read

Comment on lines 112 to 117
3) function name of a `Fn(&'static str) -> Option<&'static str>`

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

Note: This option overrides the original `TS::output_path` logic`!
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not really sure if we should allow users to override TS::output_path, this may lead to way too many footguns

Copy link
Author

Choose a reason for hiding this comment

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

things that may go wrong with this option:

  • If the path is a directory:
    a) for existing directory, the test will panic with CannotBeExported.
    b) will write into a file with no extension.
  • Anytime the function returns None, the test will panic with CannotBeExported.

Also I've found a way to propagate the error the correct message
and exact span if the provided function is of a different than required type,
by asigning first as a function pointer and then use the pointer to invoke the function:

    fn output_path() -> Option<&'static std::path::Path> {
        let path: fn(&'static str) -> Option<&'static str> = crate::get_path_fn;
        Some(std::path::Path::new(path("ObjC")?))
    } 

it is precise!

@NimonSour
Copy link
Author

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.

Comment on lines 192 to 200
// static or const
if is_screaming_snake_case(&last.to_string()) {
return Ok(CustomPath::Static(path));
}

// function
if is_snake_case(&last.to_string()) {
return Ok(CustomPath::Fn(path));
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Another reason I don't really like allowing a function path here is that there is no reliable way to differentiate between a function name and a const/static name. Sure, clippy will complain if a const isn't in SCREAMING_SNAKE_CASE, but it doesn't prevent the code from compiling

Copy link
Author

Choose a reason for hiding this comment

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

I've triyed this scenario on the new design (I've mentioned above, assignment to a function pointer ) and it looks like
it will not compile, which is not bad, this means that the macro is limited to
Rust naming conventions. As long as the mistake is excluded we are ok.

so for lower case static the error will be :

expected fn pointer `fn(&'static str) -> std::option::Option<&'static str>`
    found reference `&'static str`

@gustavo-shigueo
Copy link
Collaborator

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

@gustavo-shigueo gustavo-shigueo changed the title Proof of concept, argument to 'export_to' other than &'static str Proof of concept, argument to export_to other than &'static str Aug 29, 2024
@NyxCode
Copy link
Collaborator

NyxCode commented Aug 29, 2024

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
Copy link
Collaborator

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
Copy link
Author

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
Copy link
Author

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.

  1. 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
Copy link
Author

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

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.

3 participants