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

Revise API for manual exports #263

Merged
merged 8 commits into from
Mar 13, 2024
Merged

Conversation

NyxCode
Copy link
Collaborator

@NyxCode NyxCode commented Mar 10, 2024

This is the next revision on our journey to improve path handling πŸ˜…

  • Make public API more consistent and more powerfull:

    Function Includes Dependencies To
    TS::export ❌ TS_RS_EXPORT_DIR
    TS::export_all βœ”οΈ TS_RS_EXPORT_DIR
    TS::export_all_to βœ”οΈ custom

    Before, we had

    • TS::export, which is now TS::export_all and
    • TS::export_to, which kept its name got removed

    Aditionally, this PR adds TS::export and TS::export_all_to.

  • To make this work, I (again) changed TS::output_path.
    After the previous PR, it returned TS_RS_EXPORT_DIR + export_to.
    The same functionality is now in the function TS::default_output_path(), while the new TS::output_path() now only returns export_to, without reading any environment variables

  • I transfered that new naming scheme for the functions to export.rs.

@escritorio-gustavo I'd love to hear what you think about this.
I took care in writing nice docs for it, and I think this is overall a good change.

Todos

  • TS::export_to: Imports are generated useing the "default" TS_RS_EXPORT_DIR + export_to path, instead of the one the user provided
  • Remove TS::export_to
  • Can generate_imports be simplified?
    • No, we actually need the output directory in case a type wants to import from a parent directory.
  • TS::export_to_string: Can/should we enable formatting for this?
    • Seems like we can't, since our formatter needs a path to be provided. We could use the default path, but since I'm not sure why it wants a path, I left it as-is.
  • Is there a better name for TS::output_path() and TS::default_output_path()?

NyxCode added 2 commits March 10, 2024 17:52
Introduce missing functions "export(_all)?(_to)?"
Make naming more consistent
@escritorio-gustavo
Copy link
Contributor

Okay, my internet is failing a lot here. I submitted a review with three comments... The Github GUI showed six, duplicating all of them... I deleted the duplicates, and that deleted the originals as well 🀦

}

/// Manually export this type into the given directory, together with all of its dependencies.
/// To export only this type, without its dependencies, use [`TS::export`].
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor nitpick: I think this doc comment should link to TS::export_to instead of TS::export because it better corresponds to what is being described here (same behavior, but no dependencies)

Comment on lines 119 to 124
// TODO: this generates imports, but disregards 'path'!
// In general, imports won't work anymore if users call `TS::export_to`, and manually
// specify each file instead of using `#[ts(export_to = "..")]`.
// However, if users only do that with types that no other type depends on, then imports
// should still work! To do that, we'd need to pass `path` down until we get it into
// `generate_imports`.
Copy link
Contributor

@escritorio-gustavo escritorio-gustavo Mar 11, 2024

Choose a reason for hiding this comment

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

What if we didn't expose functions to allow the user to choose the path? Always have them choose the path #[ts(export_to = "...")] while still them to export manually at runtime export and export_all. This would also eliminate the need for TS::default_output_path and TS::output_path. If the user wishes to break out of TS_RS_EXPORT_DIR, they can always do #[ts(export_to = "../path")]

I get that this would make the less powerful, but would save a lot of trouble and possible path resolution bugs that could come from allowing the user options they used in the attributes and environment variables and maintain support for these features as more things are added.

Take this problem you mention here; how would we ever from a type T know to what path a dependency U was exported if U ignored both TS_RS_EXPORT_DIR and #[ts(export_to = "...")] at runtime? Maybe some kind of HashMap that maps types to paths? (IIRC we do have something like this to keep track of dependencies, right?) But that has some problems:

  • What should we do to this possible HashMap if the user does both export and export_to? Which path is preseerved?

  • What happens if the type is located as a dependency before being exported (and therefore before being added to the HashMap)? Do we use the path export_all would use? What if later this type is exported with export_to

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hm..

So from the 4 functions (export, export_to, export_all, export_all_to), only export_to breaks dependency resolution if used incorrectly.

If a user does SomeType::export_to("./some/where.ts"), then that export should be correct. That's what this TODO was about. However, if a user does

SomeType::export_to("....");
OtherType::export_to("....");

and these types depend on each other, then OtherType::export_to has no idea where we put SomeType.

In that sense, export_to does seem a bit useless, I agree. It only makes sense if

  • There are no dependencies to break or
  • The user only uses it on types on which no other types depend on

Since export, export_all and export_all_to all make sense, it felt right to also expose export_to.
Maybe, instead of removing it, we could note in the docs that it breaks imports?

If you think it's a bigger footgun than it does good, we can also remove it. I guess the functionality is still there with std::fs:.write("....", SomeType::export_to_string()). The best reason I have for including it is that it's a nice symmetry to have all 4.


If we wanted to really support users chosing a path for each of their types at runtime, we'd need something like SingleFileExporter or the old export!. The user provides a list of all the types he wishes to export, and where there should go.

Personally, I don't see a big need for it. Can't think of a single good usecase where you can't achieve the same with export and export_all and export_all_to.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we wanted to really support users chosing a path for each of their types at runtime, we'd need something like SingleFileExporter or the old export!. The user provides a list of all the types he wishes to export, and where there should go.

Agreed, if we allow users to do this at runtime and not all at once, we lose track of where each type is

So from the 4 functions (export, export_to, export_all, export_all_to), only export_to breaks dependency resolution if used incorrectly.

This is true, but if export_all_to is used incorrectly it will generate a lot of duplicate files right? I'm not sure on this point though. Anyways, if it does, that can lead to multiple different paths from which a user can import their type in TS. That doesn't necessarily break anything, but it does make maintenance harder. Still, this isn't that big a deal, and I'd still be fine with export_all_to, but I do agree that export_to should be removed or at least have a very large warning in the docs about breaking imports (I dislike this option, we shouldn't have a public function that breaks things)

Copy link
Contributor

Choose a reason for hiding this comment

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

I would also prefer if export_all_to didn't ignore TS_RS_EXPORT_DIR. If a user really wants to export outside of that directory they can do T::export_all_to("../some_path").

This removes the default_output_path naming problem, as we'd go back to just having output_path

Copy link
Collaborator Author

@NyxCode NyxCode Mar 11, 2024

Choose a reason for hiding this comment

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

Alright, let's get rid of export_to. Not much is lost there anyway (see fs::write("..", Type::export_to_string());).

I don't think export_all_to ever breaks. Sure, if the user does A::export_all_to("./a") and B::export_all_to("./b"), then he might get some dependencies duplicated, but that's just the expected behaviour, I'd say.

Same thing happens if the user runs both TS_RS_EXPORT_DIR=./a cargo test export_bindings_a and TS_RS_EXPORT_DIR=./b cargo test export_bindings_b. (they are literally equivalent, since the test just runs A::export_all_to(TS_RS_EXPORT_DIR))
I don't think there's an expectation that this should do anything other than duplicate files.

Copy link
Contributor

Choose a reason for hiding this comment

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

Only thing I dislike about export_all_to is this:

   /// Unlike [`TS::export_all`], this function disregards `TS_RS_EXPORT_DIR`, using the provided
   /// directory instead.

I think we shouldn't disregard TS_RS_EXPORT_DIR, that would make this path relative to Cargo.toml again. If that's what the user wants, they should use T::export_all_to("../path") or T::export_all_to(std::env::current_dir()?.join("path"))

Copy link
Collaborator Author

@NyxCode NyxCode Mar 11, 2024

Choose a reason for hiding this comment

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

Hm, I think I'd disagree with that!

I feel like TS::export_all_to("./some_path") should behave like std::fs::write - you just give it a path, and it puts the files there.
That also makes the difference between export_all and export_all_to more clear, I think: export_all is just export_all_to(TS_RS_EXPORT_DIR).

Using environment variables is a kinda sneaky way of passing parameters around. We need it for #[ts(export)], since there is no way of actually passing a parameter to the tests.
But if a user is in a situation where they do need something custom (e.g getting the path from a config file or database), I think we should not let our default (using TS_RS_EXPORT_DIR) get in their way.

Happy to hear arguments against it tho! πŸ˜„

Copy link
Contributor

@escritorio-gustavo escritorio-gustavo Mar 11, 2024

Choose a reason for hiding this comment

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

I feel like TS::export_all_to("./some_path") should behave like std::fs::write - you just give it a path, and it puts the files there.

I see. Hadn't looked at it that way, I was thinking about how it was annoying before when #[ts(export_to = "...")] ignored TS_RS_EXPORT_DIR, making it very annoying to, say, rename the file while keeping it in the right directory. Your point does make a lot of sense though

I think we should not let our default (using TS_RS_EXPORT_DIR)

I also saw it as TS_RS_EXPORT_DIR being the user's default, as they have to manually set it via .cargo/config.toml or the command line. (This might be one of those things that, whichever way we do, half of our users will complain xD)


Either way I don't feel very strongly about this, so we can definitely go for the std::fs::write behavior! As a user of ts-rs I never export types manually, so take my opinions on this with a massive grain of salt.

Copy link
Collaborator Author

@NyxCode NyxCode Mar 11, 2024

Choose a reason for hiding this comment

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

That's a good point, I hope use-cases for these functions are rare.

That being said, I think this all makes a lot of sense now:

  • #[ts(export)] and TS::export_all use TS_RS_EXPORT_DIR as base directory and
  • with TS::export_all_to, you manually supply the base directory

I was thinking about how it was annoying before when #[ts(export_to = "...")] ignored TS_RS_EXPORT_DIR

I see! Since the paths specified using #[ts(export_to)] are now always relative to some base directory, this is way more sensible now. You can organize your types in directories however you like, and then export them to TS_RS_EXPORT_DIR, or to somewhere else using TS::export_all_to.

Before, setting #[ts(export_to)] reset the base-directory to ., which was very unintuitive. So I think it's nice that these two things are now decoupled.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can organize your types in directories however you like, and then export them to TS_RS_EXPORT_DIR, or to somewhere else using TS::export_all_to.

Awesome, this cleared it up a lot for me!

generate_decl::<T>(&mut buffer);
Ok(buffer)
}

pub(crate) fn default_out_dir() -> Cow<'static, Path> {
match std::env::var("TS_RS_EXPORT_DIR") {
Err(..) => Cow::Borrowed(Path::new("./bindings")),
Copy link
Contributor

Choose a reason for hiding this comment

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

Cool, I didn't know Err(..) was valid syntax to ignore enum data, I though it had to be Err(_)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's a character more, but I kinda like how it looks πŸ˜†

Comment on lines 139 to 146
{
// Manually write to file & call `sync_data`. Otherwise, calling `fs::read(path)`
// immediately after `T::export()` might result in an empty file.
use std::io::Write;
let mut file = File::create(path)?;
file.write_all(buffer.as_bytes())?;
file.sync_data()?;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

So that's why I was getting some weird empty files when adding #[ts(export)] to some tests xD

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

possibly :D
I noticed it in tests/imports.rs, where line 68 would panic with an empty string.

@NyxCode NyxCode marked this pull request as ready for review March 13, 2024 11:49
@NyxCode
Copy link
Collaborator Author

NyxCode commented Mar 13, 2024

From my side, this is ready for merge @escritorio-gustavo

@escritorio-gustavo
Copy link
Contributor

escritorio-gustavo commented Mar 13, 2024

our formatter needs a path to be provided. We could use the default path, but since I'm not sure why it wants a path, I left it as-is.

It seems like the formatter is only interested in the file extension, it probably formats .js, .jsx, .ts etc diferently. This could be better signalled to the caller if they exposed the MediaType enum and asked for that instead

@escritorio-gustavo
Copy link
Contributor

Even GitHub has some trouble with JS lol
image

@escritorio-gustavo escritorio-gustavo merged commit b8ff94f into main Mar 13, 2024
14 checks passed
@escritorio-gustavo escritorio-gustavo deleted the 4th-take-on-path-handling branch March 13, 2024 12:05
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