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

Alternative handling of output paths #256

Merged
merged 3 commits into from
Mar 7, 2024

Conversation

NyxCode
Copy link
Collaborator

@NyxCode NyxCode commented Mar 7, 2024

Hey @escritorio-gustavo!

This is an experiment to see if we can further simplify and streamline the handling of export paths.
Let's start with a practical example:

Example

Before

In the workspace E2E test, there's the parent crate, which depends on crate1.
The struct crate1::Crate1 has #[ts(export_to = "crate1/")], and
the struct parent::Parent has #[ts(export)].

When running cargo test, only parent::Parent exports, but it takes its dependency crate1::Crate1 with it.
That results in this directory structure:

parent/
    bindings/
        Parent.ts
    crate1/
        Crate1.ts

After

With this change, the resulting directory structure looks like this:

parent/
    bindings/
        Parent.ts
        crate1/
            Crate1.ts

Explanation

Before

For a type, the output path is
./{TS_RS_EXPORT_DIR}/bindings/{name}.ts if #[ts(export_to = "...")] is not specified and
./{TS_RS_EXPORT_DIR}/{name}.ts if #[ts(export_to = "...")] is specified.

This is because TS::EXPORT_TO defaults to bindings/{name.ts}, but changing it using #[ts(export_to = "...")] gets rid of the bindings/ prefix.
The environment variable TS_RS_EXPORT_DIR defaults to ..

After

With this change, it's the environment variable TS_RS_EXPORT_DIR which defaults to ./bindings.

Therefore, with this change, the output path is always ./{TS_RS_EXPORT_DIR}/{name}.ts.
We take the TS_RS_EXPORT_DIR (or ./bindings), append the name to it (or the #[ts(export_to = "...")] override), and we're done.


I feel like this behaviour better matches what users would expect. They specify where there files should go relative to some base directory, TS_RS_EXPORT_DIR.
What do you think?

Comment on lines +407 to +420

/// Returns the output path to where `T` should be exported.
///
/// When deriving `TS`, the output path can be altered using `#[ts(export_to = "...")]`.
/// See the documentation of [`TS`] for more details.
///
/// The output of this function depends on the environment variable `TS_RS_EXPORT_DIR`, which is
/// used as base directory. If it is not set, `./bindings` is used as default directory.
///
/// If `T` cannot be exported (e.g because it's a primitive type), this function will return
/// `None`.
fn output_path() -> Option<PathBuf> {
None
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Regarding the implementation: I moved output_path() into TS, and completely got rid of the const EXPORT_TO.

Comment on lines +47 to +52
fn output_path() -> Option<std::path::PathBuf> {
let path = std::env::var("TS_RS_EXPORT_DIR");
let path = path.as_deref().unwrap_or("./bindings");

Some(std::path::Path::new(path).join(#path))
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The implementation of that is super simple - Take TS_RS_EXPORT_DIR (or ./bindings), and append the name (or #[ts(export_to = "...")] override to it.

@NyxCode
Copy link
Collaborator Author

NyxCode commented Mar 7, 2024

In our example, cargo test does the same as before:

example/
  bindings/
    ...

With TS_RS_EXPORT_DIR=./typescript, however, you'd get this:

example/
  typescript/
    bindings/
      ...

With this change, you'd get

example/
  typescript/
    ...

Comment on lines 10 to 15
#[derive(Serialize, TS)]
#[ts(rename_all = "lowercase")]
#[ts(export, export_to = "bindings/UserRole.ts")]
#[ts(export, export_to = "UserRole.ts")]
enum Role {
User,
#[ts(rename = "administrator")]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Before, renaming the file from Role.ts to UserRole.ts required adding the bindings/ prefix back again.
With this change, this is no longer necessary.

@escritorio-gustavo
Copy link
Contributor

escritorio-gustavo commented Mar 7, 2024

For a type, the output path is
./{TS_RS_EXPORT_DIR}/bindings/{name}.ts if #[ts(export_to = "...")] is not specified

Oof, this bindings is not supposed to be there, this is a major oversight from my part when doing #250, caused by the fact that all of our tests use #[ts(export_to = "...")], so I didn't even consider what would happen without it

@escritorio-gustavo
Copy link
Contributor

After

With this change, it's the environment variable TS_RS_EXPORT_DIR which defaults to ./bindings.

Therefore, with this change, the output path is always ./{TS_RS_EXPORT_DIR}/{name}.ts. We take the TS_RS_EXPORT_DIR (or ./bindings), append the name to it (or the #[ts(export_to = "...")] override), and we're done.

Very nice way of dealing with it!

@NyxCode
Copy link
Collaborator Author

NyxCode commented Mar 7, 2024

Alright, then I'll go ahead and merge this!
It is a breaking change, but it's probably a good time to do it.
All the tests, for example, have #[ts(export_to = "tests_out/")], so they end up in ts-rs/bindings/tests-out.

@NyxCode NyxCode marked this pull request as ready for review March 7, 2024 18:19
@NyxCode NyxCode merged commit cd59b2c into main Mar 7, 2024
10 checks passed
@escritorio-gustavo escritorio-gustavo deleted the alternative-take-on-path-handling branch March 7, 2024 18:20
@escritorio-gustavo
Copy link
Contributor

escritorio-gustavo commented Mar 7, 2024

All the tests, for example, have #[ts(export_to = "tests_out/")]

We could make a separate PR to remove tests-out from the tests to reduce nesting

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.

2 participants