TS_RS_EXPORT_DIR quirks

[escritorio-gustavo]

Hey @NyxCode! There are a few things related to the TS_RS_EXPORT_DIR environment variable and #[ts(export_to = "...")] that I'd like to discuss. Almost all of which are breaking changes:

• We need a way to clean paths containing .. as that can cause problems in diff_paths (done in bug: #[ts(export_to = "../path")] can cause diff_paths to fail #247)
• When we use TS_RS_EXPORT_DIR and #[ts(export_to = "...")] the path in #[ts(export_to = "...")] is still relative to Cargo.toml, and not to TS_RS_EXPORT_DIR. Should we change this? (done in Make #[ts(export_to = "...")] relative to TS_RS_EXPORT_DIR #250 + Alternative handling of output paths #256)
• We determine if the path given to #[ts(export_to = "...")] is a directory by whether or not it ends in a slash /. I think we should change this to detect a .ts file extension instead because:
  • It's way too easy to forget
  • Not all operating systems use / as the directory separator (Windows accepts it, but it actually uses the backslash \)

[NyxCode]

When we use TS_RS_EXPORT_DIR and #[ts(export_to = "...")] the path in #[ts(export_to = "...")] is still relative to Cargo.toml, and not to TS_RS_EXPORT_DIR. Should we change this?

Hm, you're right! That's very unintuitive! I think we should definetely change this!
Since we haven't published a release with TS_RS_EXPORT_DIR yet, now is the best time to fix this.

We determine if the path given to #[ts(export_to = "...")] is a directory by whether or not it ends in a slash /. I think we should change this to detect a file extension

This tripped me up a couple of times as well. I do think the current behaviour is "correct" and how many tools handle this, but it's a bit unintuitive, since exporting to a file without extension is most likely not what any user wants, ever.

If we changed this, it would become impossible to export to a file without extension though. Probably not a (common) usecase, but it's still something which would become completely impossible.
So yeah, on this, I don't have a strong opinion. If I had to decide, then I'd keep it as-is, but I wouldn't be against changing it either.

We need a way to clean paths containing .. as that can cause problems in diff_paths
We could use the path-clean crate for this

Do you have an example for this?

[escritorio-gustavo]

Do you have an example for this?

I do remember @stegaBOB mentioning issues with it back in #195. I'll check if I can reproduce it

[escritorio-gustavo]

If we changed this, it would become impossible to export to a file without extension though. Probably not a (common) usecase, but it's still something which would become completely impossible.

We could make a trailing . mean a file without an extension, since no file can have a trailing dot, this should be a hacky, but working solution

I don't see why anyone would ever want a file without the .ts extension though, since that would make it very hard for tsc to pick up and type check that file

If I had to decide, then I'd keep it as-is

That's fine with me too, having to use the / at the end is not that big a deal. I do think we should also support \ though

[escritorio-gustavo]

Do you have an example for this?

I do remember @stegaBOB mentioning issues with it back in #195. I'll check if I can reproduce it

Got it! Check out #247

[NyxCode]

Got it! Check out #247

Ah, thanks for reproducing it!
If, for example, base is ../ and path is a/, then diff_paths has no clue how to get to a/ from one level up.

However: If we started with an absolute path (by e.g doing var("CARGO_MANIFEST_DIR")?.join(path)), then we'd get something like /home/a/b/c/../. Here, it'd be possible to get rid of the ../ component, since it would cancel itself out with the c/ component before it.

There's std::path::absolute on nightly which does that, but I think we can implement a crappy version ourself.

[escritorio-gustavo]

I think we can implement a crappy version ourself.

I just pushed a commit to do that. However, I think we should change diff_paths to return Result, because to keep the Option I had to ignore an error

[NyxCode]

Noooooooo, you were faster than me, again! 😆

[escritorio-gustavo]

There's std::path::absolute on nightly

Man, it feels like everything we need to make our lives easier is in nightly lol

[NyxCode]

welcome to rust 😂

[escritorio-gustavo]

I think it's a better idea to use std::env::current_dir than CARGO_MANIFEST_DIR, as that's how it is currently implemented in std::path::absolute

[NyxCode]

I think it's a better idea to use std::env::current_dir than CARGO_MANIFEST_DIR, as that's how it is currently implemented in std::path::absolute

Hmm. I felt like CARGO_MANIFEST_DIR makes more sense, since that's what the paths are actually supposed to be relative to. But as far as I can tell, CARGO_MANIFEST_DIR == current_dir() when running cargo test, no matter what I try. So maybe it doesn't matter in this use-case?

[escritorio-gustavo]

But as far as I can tell, CARGO_MANIFEST_DIR == current_dir()

Looks that way to me too. Maybe the test binary is always executed from CARGO_MANIFEST_DIR?

[escritorio-gustavo]

Found a difference! In #248 CARGO_MANIFEST_DIR causes CI to fail because the types in the example directory go to ts-rs/bindings instead of example/bindings

[escritorio-gustavo]

If we changed this, it would become impossible to export to a file without extension though. Probably not a (common) usecase, but it's still something which would become completely impossible.

We saw in this test run (which was fixed by 6c3b0cb) that files without extensions already break exporting when the format feature is enabled, so is this really a problem?