Always convert to absolute path before calculating diff

[escritorio-gustavo]

Convert path and base into absolute paths before diffing the two

Fixes #111

[escritorio-gustavo]

I used this StackOverflow answer to implement path conversion, but it seems to me that the current diffing logic already does the job of the path-clean crate, so I did not add it. Please correct me if I am mistaken about this

[NyxCode]

Code-wise, this looks alright! I'd have to write a few tests to really understand what currently goes wrong, and how this fixes it though. Maybe you could provide some info on this.

[escritorio-gustavo]

This one is a bit hard to test, since it fixes a weird issue that occurs when dealing with workspaces, demonstrated by @h-michael's example project. Basically all the bindings would be correctly exported to the bindings directory, but the import statements in the TS files would contain the ../ path components used in #[ts(export_to)], even though they shouldn't, given that all TS files are in the same directory.

I've been testing against that, and I recall being sucessful on Friday, though today it seems the imports in ABC.ts are wrong again, I'll need to check why that is

[NyxCode]

Ah, awesome, missed that test repository! I think now I understand the issue! Might be cool to be able to reproduct it in a unit test, right in export.ts. My gut tells me that we have a .. too much in some cases. Without that, import type { A } from "../../bindings/A" should turn into import type { A } from "../bindings/A", which should turn into import type { A } from "./A".

[NyxCode]

Let me know if you get stuck, happy to take a look at it.

[escritorio-gustavo]

I think I know what the problem is, but I'm still not quite sure how to solve it. Given the project structure:

.
├── bindings
├── api
│   ├── Cargo.toml
│   └── src
│       └── abc.rs  - exports to `../bindings`
├── dep_a
│   └── lib_a
│       ├── Cargo.toml
│       └── src
│           └ a.rs  - exports to `../../bindings`
└── dep_b
    └── lib_b
        ├── Cargo.toml
        └── src
            └ b.rs  - exports to `../../bindings`

All the export paths are relative to their respective Cargo.toml, which is needed to export properly. But when the api project reads these paths, they are not relative to its Cargo.toml, which causes abc.rs to produce an invalid import path

[escritorio-gustavo]

I have found that the command cargo locate-project --workspace gives the path to the workspace's Cargo.toml, and when not in a workspace, the project's Cargo.toml. We could make relative paths relative to that, but that would be a breaking change for anyone using workspaces

[escritorio-gustavo]

In @h-michael's case, all of his exports would be #[export_to = "./bindings/"]

[escritorio-gustavo]

Another issue with this approach: the command returns a json string ({"root":"C:\\Users\\...\\ts-rs\\Cargo.toml"}) , so we would have to either

• take a dependency on serde_json or
• .trim_start_matches(r#"{"root":""#).trim_end_matches(r#""}"#) which is very hacky

[NyxCode]

Another issue with this approach: the command returns a json string ({"root":"C:\\Users\\...\\ts-rs\\Cargo.toml"}) , so we would have to either

• take a dependency on serde_json or
• .trim_start_matches(r#"{"root":""#).trim_end_matches(r#""}"#) which is very hacky

There's --message-format plain, which is what we want, i think.

I guess we should decide if we want to make everything relative to the workspace root. It's probably the better choice, but, as you mentioned, might be bothersome to the users. I'm not sure how big of a deal this would be, since the current state is somewhat broken anyway.

That being said, I think we could also fix the current implementation. I'm not 100% sure yet how, but:
We create the import path here. Here, we might want to modify either from or import to reflect that these two paths are relative, but not relative to the same directory.
We'd still need a way to figure out how to get the "crate path" of a given Dependency.

I guess I'd be fine with either option, but this is probably something we want to put some thought into.

[escritorio-gustavo]

There's --message-format plain, which is what we want, i think.

Just checked, you're right, that gives the path in plaintext

We'd still need a way to figure out how to get the "crate path" of a given Dependency.

This seems really hard to do. When cargo test gets to the api project, it runs whatever it needs from dep_a and dep_b, using its own Cargo.toml as the root, which gets to Dependency and eventually breaks the import path. I am not sure there is a way to get a diferent crate's root path to allow this to work

[escritorio-gustavo]

There's --message-format plain, which is what we want, i think.

We should also add --quiet to avoid warnings and notes about resolver versions

[NyxCode]

We'd still need a way to figure out how to get the "crate path" of a given Dependency.

This seems really hard to do. When cargo test gets to the api project, it runs whatever it needs from dep_a and dep_b, using its own Cargo.toml as the root, which gets to Dependency and eventually breaks the import path. I am not sure there is a way to get a diferent crate's root path to allow this to work

I might not have thought this through 100%, but wouldn't this work?

• The #[derive(TS)] macro get's executed for each type within each crate.
• In that macro invocation, we've got access to CARGO_MANIFEST_DIR, which should give us the path to the crate
• When we generate the impl TS for T { .. } impl, we add a const CRATE_PATH: &'static str.
• We add a crate_path: &'static str to Dependency as well, and during Dependency::from_ty<T>(), we set it to T::CRATE_PATH

We should also add --quiet to avoid warnings and notes about resolver versions

Probably a good idea, though that might end up in stderr, idk

[escritorio-gustavo]

As far as I've been able to test/understand (which, to be honest, is not very far), this doesn't work because when api goes to get the import path, all the code executes taking api as the root. The const is also problematic because you can't assign a const to something that reads the file system

Again, I haven't completely wrapped my head around this quite yet, so I may very well be wrong, but it looks like this is what's happening

[escritorio-gustavo]

In that macro invocation, we've got access to CARGO_MANIFEST_DIR, which should give us the path to the crate

I haven't tried anything using this. That might actually work

[escritorio-gustavo]

I tried this today, and after a few failed attempts with commically incorrect results I actually got it to work, but it does require cleaning up the paths with something like path-clean

[escritorio-gustavo]

but it does require cleaning up the paths with something like path-clean

Path::canonicalize seems to work just fine, but it actually reads from the file system (it runs File::open) which means it may not be ideal

[escritorio-gustavo]

@NyxCode, I think we have four options here

• Use canonicalize anyway
• Copy path-clean's implementation
• Take a dependency on path-clean
• Somehow change diffing logic to deal with a bunch of ../ components

[NyxCode]

@escritorio-gustavo Awesome, thank you so much for the research on this. To get a full picture of this issue (and the related #177), I think I'll setup some end-to-end tests for the different scenarios.

Let's say my crate MyRestAPI is depending on some other crate which implements TS on some of its types. If MyRestAPI has structs which include this type, the dependencies are broken. This is what I understand #177 to be about.

I'd really like a solution which works for both workspaces and the scenario I just described.

[escritorio-gustavo]

Oh sorry, I missed this bit of #177

"CoolEnum" also has the correct annotations, but is being imported from another crate

Which made me think it was about the #[ts(as = "...")] attribute

[escritorio-gustavo]

I think I'll setup some end-to-end tests for the different scenarios.

Thanks, due to the fact that this whole problem relies on having multiple crates involved I'm really struggling to come up with a way to test it

[escritorio-gustavo]

Which made me think it was about the #[ts(as = "...")] attribute

Still... can't as solve the external crate case?

[escritorio-gustavo]

Still... can't as solve the external crate case?

Something like this:

#[derive(TS)]
#[ts(export)]
pub struct LibraryTypeDef {
    pub a: i32
}

pub struct MyStruct {
    #[ts(as = "LibraryTypeDef")]
    pub foo: other_crate::LibraryType
}

[NyxCode]

Absolutely, that'd work as a workaround. But If other_crate::LibraryType already implements TS, it'd be nice to be able to just use that.

[escritorio-gustavo]

It really would be nice, but should library code in crates.io ever implement TS? Seems like this should be an application code decision (especially the extra dependency + tests)