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

Feat: add default export directory override #195

Merged

Conversation

stegaBOB
Copy link
Contributor

@stegaBOB stegaBOB commented Jan 21, 2024

Adds the option to set a default export_to directory by setting a TS_RS_EXPORT_DIR env var.

My goal here was to keep this as backwards compatible as possible. While the Dependency struct type changed slightly, this should be relatively unobtrusive.

There's currently an issue with parent dir paths (like ../../bindings) potentially causing problems in the diff_paths function by returning None, but this already can happen with normal #[ts(export_to = )].

Closes #138

todo:

  • Add docs (Will do this if the design here is okay by a maintainer!)

@NyxCode
Copy link
Collaborator

NyxCode commented Jan 22, 2024

Thanks for the PR, I really appreciate it! At a first glance, everything looks good.

I will take a closer look in the coming days when I find some time. It has been a while since I worked on that part of the codebase, so I'll have to put some time aside for that.

@escritorio-gustavo escritorio-gustavo added the enhancement New feature or request label Jan 26, 2024
escritorio-gustavo added a commit that referenced this pull request Feb 1, 2024
@escritorio-gustavo
Copy link
Contributor

@NyxCode, I fixed the merge conflict, but maybe you wanna have a look to make sure

@escritorio-gustavo
Copy link
Contributor

Add docs (Will do this if the design here is okay by a maintainer!)
There's currently an issue with parent dir paths

@stegaBOB when you get to documenting this, make sure to show this:

  1. Create the file: <project-root>/.cargo/config.toml
  2. Add the following to the file:
[env]
TS_RS_EXPORT_DIR = { value = "<RELATIVE_PATH_FROM_PROJECT_ROOT_TO_BINDINGS_DIR>", relative = true }

Doing this will make cargo convert any relative path into an absolute path automatically, eliminating the issue you mentioned

@stegaBOB
Copy link
Contributor Author

stegaBOB commented Feb 1, 2024

Thanks @escritorio-gustavo! I'll have the docs done by this weekend

@stegaBOB
Copy link
Contributor Author

stegaBOB commented Feb 6, 2024

Hi! Sorry for the delay here. I was messing a bit with the export path resolver code since I wasn't fully satisfied with the absolute route. It seems like even with relative = true, there are cases that fail unfortunately, particularly when an import path is relative and the base is absolute.

#[derive(Serialize, TS)]
#[ts(export, export_to = "bindings/")]
enum Inner {
    Inner,
}

#[derive(Serialize, TS)]
#[ts(export, export_to = "../stuff/")]
struct Outer {
    inner: Inner,
}

This fails because the path starts with the ParentDir component and it doesn't know how to compare with bindings. I went ahead and got fixes for this issue working locally. It tries to resolve parent paths with fs and then normalizes them with what it is comparing to. My code is super rough at the moment, but I'll clean that up and open a PR soon.

I'll try and actually finish the documentation on this PR tomorrow! 😅

@escritorio-gustavo
Copy link
Contributor

I believe the issues you are having are caused by needing some way to clean the paths after combining them with the relative = true env variable to remove ParentDir from something like /home/foo/bar/../biz and convert it to /home/foo/biz.

Check #211 (comment) to see how I tried to tackle that issue

Note: I am only guessing this is the problem you're having, if it's not, ignore this comment

@escritorio-gustavo
Copy link
Contributor

escritorio-gustavo commented Feb 6, 2024

What I mean here is:

let env_path = std::path::Path::new("/home/foo/bar/"); // Resulting path from config.toml
let export_to = env_path.join("../biz");

println!("{}", export_to.to_string_lossy()); // output: "/home/foo/bar/../biz"

This will break diff_paths unless you manage to cleanup the path to be /home/foo/biz

@escritorio-gustavo
Copy link
Contributor

Make sure to write tests for this feature using the e2e directory and add the appropriate CI jobs to run your tests in .github/workflows/test.yml

@stegaBOB
Copy link
Contributor Author

stegaBOB commented Feb 7, 2024

What I mean here is:

let env_path = std::path::Path::new("/home/foo/bar/"); // Resulting path from config.toml
let export_to = env_path.join("../biz");

println!("{}", export_to.to_string_lossy()); // output: "/home/foo/bar/../biz"

This will break diff_paths unless you manage to cleanup the path to be /home/foo/biz

Ah! I missed #211. Basically my idea was a super convoluted version of what you did, but with an added parent dir resolver on top. Plopping my resolver into your branch just makes it work (without the rest of my garbage code along with it haha).

fn resolve_parent_dirs(path: &Path) -> PathBuf {
    let mut resolved = PathBuf::new();

    for component in path.components() {
        match component {
            Component::ParentDir => {
                // when dealing with absolute paths, there should always be a component to pop off
                resolved.pop();
            }
            Component::CurDir => {}
            _ => {
                resolved.push(component);
            }
        }
    }
    resolved
}

I didn't fully read the entire thread there, but it looks like there were additional issues with different libraries that this wouldn't fix ☹️

@stegaBOB
Copy link
Contributor Author

stegaBOB commented Feb 7, 2024

I added some docs! Included the config.toml tip as well. It might take me a few more days to get tests done (should be done by the end of this weekend though). Super busy this week!

edit: Just accidentally clicked the close button! Whoops

@stegaBOB stegaBOB closed this Feb 7, 2024
@stegaBOB stegaBOB reopened this Feb 7, 2024
@escritorio-gustavo
Copy link
Contributor

escritorio-gustavo commented Feb 7, 2024

I didn't fully read the entire thread there, but it looks like there were additional issues with different libraries that this wouldn't fix

That thread is really long because somewhere along the way I realized that was the wrong approach and we also had to support exporting types from external libs, which the approach used in #211 wouldn't be able to do (dealing with external libs has been solved already by #221 and #218, don't worry about it.).

What I did in that branch was trying to convert paths into absolute paths and use CARGO_MANIFEST_DIR. That's not the right way to go about it for the feature you're building. I only linked that specific comment to show you that there are (as I saw it at the time) 4 ways to solve the problem, all described in that comment.

I also made a different PR approaching that topic that uses your code. It worked out pretty nice, but still required some way of dealing with .. components. Check out #220.

Anyway... as far as I can tell your PR is really solid and will work nicely, the only loose end is .. components. It seems to me like your resolver can handle that issue, but if you're not sure, just use path-clean :D

@escritorio-gustavo
Copy link
Contributor

Things got very confusing at that period of time because we had like 3 or 4 PRs + 1 discussion (#219) on the same topic. Here's the TLDR:

Your PR is great, just fix the problem of .. with one of the approaches mentioned in this comment or your own approach

@escritorio-gustavo
Copy link
Contributor

I added some docs! Included the config.toml tip as well. It might take me a few more days to get tests done (should be done by the end of this weekend though). Super busy this week!

Don't worry about it, take your time

ts-rs/src/lib.rs Outdated Show resolved Hide resolved
@escritorio-gustavo
Copy link
Contributor

Great! Thank you for adding the docs and tests. Awesome work with this PR @stegaBOB!

@escritorio-gustavo escritorio-gustavo merged commit 823abfd into Aleph-Alpha:main Feb 12, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: Ability to set default export_to directory
3 participants