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

Make #[ts(export_to = "...")] relative to TS_RS_EXPORT_DIR #250

Merged
merged 11 commits into from
Mar 7, 2024

Conversation

escritorio-gustavo
Copy link
Contributor

As discussed in #245, when using both TS_RS_EXPORT_DIR and #[ts(export_to = "...")], the export_to path should be relative to TS_RS_EXPORT_DIR instead of the project's Cargo.toml

@escritorio-gustavo escritorio-gustavo marked this pull request as draft March 6, 2024 17:23
@escritorio-gustavo
Copy link
Contributor Author

This is so annoying, docs.rs passes on my computer, but not on ci

@escritorio-gustavo
Copy link
Contributor Author

That's weird, changing the order of the tests changed which one failed

@NyxCode
Copy link
Collaborator

NyxCode commented Mar 6, 2024

I don't quite understand what's going on, but it seems like TS_RS_EXPORT_DIR is not respected.

in export_type, we call output_path, but that doen't take TS_RS_EXPORT_DIr into account at all.

@NyxCode
Copy link
Collaborator

NyxCode commented Mar 6, 2024

Running TS_RS_EXPORT_DIR=./custom cargo test -- export_bindings still puts everything in tests-out

let export = self
.export
.then(|| self.generate_export_test(&rust_ty, &generics));

let export_dir = std::env::var("TS_RS_EXPORT_DIR").ok();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ahh, I think this here might be the issue!
This is evaluated when the macro executes, during compilation.

When later running this executable, the TS_RS_EXPORT_DIR is hard-coded in there.
That might also happen when just doing cargo test, since rust might not re-compile.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you're right, I just commited a version using std::option_env! instead and that seems to have fixed it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Still, it might be better to move reading the env variable to export.rs instead, because I think the way I just did it forces recompilation, which kinda sucks

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed, both with the observation as well as the conclusion.

Copy link
Collaborator

@NyxCode NyxCode Mar 6, 2024

Choose a reason for hiding this comment

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

The last change did cause a recompilation, but recompilation of ts-rs-macros.
The 3 possible behaviours we could get are

  • Re-compile ts-rs-macros (which also re-compiles the consumer crate)
    Use env! in ts-rs-macros
  • Re-compile the consumer crate
    Load the environment variable in export.rs, using env!()
  • Don't recompile anything
    Load the environment variable in export.rs, using env::var()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it fixed! Sorry about the billion failed CI notifications 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • Don't recompile anything
    Load the environment variable in export.rs, using env::var()

Ended up on this one, my cpu is already thanking me 😆

@escritorio-gustavo
Copy link
Contributor Author

I finished moving std::env::var outside of the macro. The only downside to this is that T::EXPORT_TO doesn't actually correspond to the exported path anymore, due to the lack of TS_RS_EXPORT_DIR, but this should reduce the amount of compilation going on in the user's machine

@escritorio-gustavo
Copy link
Contributor Author

I could add a method that returns the proper path to remedy this

Comment on lines +169 to +177
let base = std::env::var("TS_RS_EXPORT_DIR")
.ok()
.as_deref()
.map(Path::new)
.unwrap_or_else(|| Path::new("."))
.to_owned();
let export_to =
T::get_export_to().ok_or(ExportError::CannotBeExported(std::any::type_name::<T>()))?;
let path = Path::new(&export_to);
T::EXPORT_TO.ok_or(ExportError::CannotBeExported(std::any::type_name::<T>()))?;
let path = base.join(export_to);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we re-use output_path here? Might be the case that we have to get rid of the path::absolute in output_path for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't even think we need to remove path::absolute, this path will be fed into diff_paths, which uses absolute paths as well

Copy link
Collaborator

Choose a reason for hiding this comment

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

you're right, perfect!

@escritorio-gustavo
Copy link
Contributor Author

The reason I was thinking about adding it to TS is because of the tests that read the exported file, all of them have a bunch of weird stuff to handle TS_RS_EXPORT_DIR

ts-rs/src/lib.rs Outdated
Comment on lines 424 to 434
let base = std::env::var("TS_RS_EXPORT_DIR")
.ok()
.as_deref()
.map(Path::new)
.unwrap_or_else(|| Path::new("."))
.to_owned();

let exported_to = base
.join(T::EXPORT_TO.map(ToOwned::to_owned)?)
.to_string_lossy()
.to_string();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same thing here, might be nice to re-use output_path here as well

@NyxCode
Copy link
Collaborator

NyxCode commented Mar 6, 2024

Yeah, that's a good point. If you think it makes sense, go for it!

@escritorio-gustavo
Copy link
Contributor Author

escritorio-gustavo commented Mar 6, 2024

I agree that the function that returns the path should never be overwritten. Maybe a pub fn would be better than a new TS method then?

Making output_path public would probably do the trick

@escritorio-gustavo escritorio-gustavo marked this pull request as ready for review March 7, 2024 11:10
@escritorio-gustavo escritorio-gustavo merged commit 3a667d4 into main Mar 7, 2024
10 checks passed
@escritorio-gustavo escritorio-gustavo deleted the export_relative_ts_rs_env branch March 7, 2024 11:12
NyxCode added a commit that referenced this pull request Mar 7, 2024
* document `#[ts(tag)]` and `#[ts(as)]`, reformat documentation

* fix line breaks in README.md

* put cargo features in a table

* Fix typo in readme

* Document change to export_to made in #250

* Add missing newline

* Explain flatten

* Document #[ts(untagged)] on enum variants

* Document #[ts(rename_all = ..)] on enum variants

* Add missing newline

* Change interface to type

* Link to mentioned items

* Run cargo readme

* Document TS::DOCS

* Improve TS::ident so it doesn't panic

* document TypeList

---------

Co-authored-by: Gustavo <gustavo.shigueo@proton.me>
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.

None yet

2 participants