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

Avoiding the testcase workaround #62

Closed
cauthmann opened this issue Nov 23, 2021 · 7 comments
Closed

Avoiding the testcase workaround #62

cauthmann opened this issue Nov 23, 2021 · 7 comments
Labels
enhancement New feature or request P-HIGH This will be treated with high priority

Comments

@cauthmann
Copy link

Hello,

as you're aware, hiding the code generation in testcases is a bit of a hack and probably annoying for anyone actually using their tests for other matters. Like testing.

If you haven't seen it, https://github.com/Wulf/tsync is taking a different approach: Instead of generating a meaningful #[derive(TS)] at build time, it will parse your source code independently of the compiler. But both tsync and ts-rs use the syn crate, and will parse a syn::Item::Struct/::Enum into type information.

Their approach has a few advantages:

  • doesn't mess with test cases
  • no build overhead to generate meaningful #[derive(..)] (though that is probably minor)
  • it's easier to regenerate .d.ts files on demand (just run tsync instead of all your tests)

But let's not hide the disadvantages:

  • they currently require global installation of a tsync binary. This could be avoided by letting users add a one-liner [[bin]] or [[example]] to their project and calling that. (an example has the advantage that you can put its deps into dev-dependencies)

I was wondering if it'd be useful to better abstract the type parsing away from the type consumption. We create a function taking a syn::Item and returning a struct DerivedTS (or something more generic than it).

Using that function, we could implement both the actual #[derive(TS)] and testcase generation, but we could also implement an external parser similar to tsync for projects where that's a better fit.

Do you think that'd be useful? Am I overlooking something important? Is this something I should hack on the next slow weekend, or over the holidays?

@NyxCode
Copy link
Collaborator

NyxCode commented Nov 23, 2021

Hey!
I've just taken a look at tsync, and it's a completely different beast than ts-rs. It naively takes parsed idents and tries to convert them to TS. Here is an excerpt of the source:

match identifier.as_str() {
  "i8" => "number".to_string(),
  "u8" => "number".to_string(),
  "i16" => "number".to_string(),
  "u16" => "number".to_string(),
  ...
  "NaiveDateTime" => "Date".to_string(),
  "DateTime" => "Date".to_string(),
  ...
  _ => identifier.to_string()
}

While this probably works for simple usecases, it's really limiting since the tool has no idea what the types are - it just echoes their identifier to typescript.
This approach can't support things like renaming types, inlining types, flattening types and many more.
Also, it's easy to break - Type aliases or shadowed identifiers would just result in wrong ts being generated.

ts-rs takes a different approach, defering the actual generation of typescript to runtime. With this, we can make use of rusts name resolution/type checking to properly support all those features. It lets you flatten, inline, rename, ... your types and doesn't get confused when you use type aliases or shadow identifiers.

More concretely: DerivedTS is basically the information needed to generate an impl TS for T block. So at this point, there are no actual bindings generated, but just something like

#[derive(TS)]
#[ts(rename = "SomeType")]
struct T {
  a: i32,
  #[ts(flatten)]
  b: Something,
}

impl TS for T {
   ...
  fn name() -> String {
    "SomeType".to_owned()
  }
  fn inline() -> String {
    format!("{ a: {}, {} }", <i32 as TS>::name(), <Something as TS>::inline_flattened())
  }
  ...
}

So, tldr: ts-rs depends on doing stuff at runtime, and for good reason.
However, I'd love to make the workaround more paletable - Maybe we should gate the tests which export typescript behind a feature flag or some other way of easily disabeling them/running them. (That's currently possible too by excluding export_typescript_* tests, but it's not super obvious).

@cauthmann
Copy link
Author

Good points, thanks for taking the time to explain. You aren't just parsing the tokens, you're re-inserting the tokens into the source code to get the compiler to do important work on them. So the external parser won't work.

If we cannot pull type generation out of the code, I'll try to find a workaround with feature gates. The goal is that

  • running tests doesn't generate typescript
  • generating typescript doesn't run other tests
  • no deps or features are introduced into the app. proc-macros and dev-dependencies are fine, as cargo can isolate them.

I'll play around with it as time permits and check back with you.

@NyxCode
Copy link
Collaborator

NyxCode commented Nov 23, 2021

Awesome, thanks. Really apreciate it.
Regarding point 1 & 2: Filtering by name (currently the tests start with export_bindings i think), using #[ignore] and cargo test -- --ignored, or some conditional compilation mechanism comes to mind.

@cauthmann
Copy link
Author

Success.

  • ts-rs-macros gets a feature called export-bindings. If not set, generate_export_test does nothing, and no tests are generated.
  • ts-rs gets the same feature, which is passed through to ts-rs-macros. If not set, TS::export does not exist.

Running cargo test does not generate bindings, as the feature isn't set. Keeping this working without requiring additional parameters is good for those using the cli.

Running cargo test --features "ts-rs/export-bindings" export_bindings_ runs just the exports, but not any other tests. That's a bit long, but it would go into a build script.

dprint-plugin-typescript can be made optional, depending on export-bindings. That might conflict with #58 - I don't think we can enable a dependency only when two other features (export-bindings + pretty-print) are enabled. But for the goal of keeping dprint out of the app's dependency graph, maybe this is enough?

This works best if the feature "export_bindings" is not set by default, but that's a breaking change. Otherwise we'd have to tell users to manually disable the feature when specifying the ts-rs dep.

But I'm wondering, how far should we go here? Right now, the macro derives are still generated outside of tests. We could disable that to avoid the compile time penalty. Are you aware of any users consuming the traits other than for exporting? Depending on the answer, we might turn the whole crate inert when export-bindings is off, or we could gate trait generation behind a second feature flag. Or we could leave it as is.

@Heliozoa
Copy link
Contributor

Heliozoa commented Dec 1, 2021

You can also opt for a more manual approach and write lines of format!("export {}", MyType::decl()) into a file yourself in any function that you like, without any export annotations. I wrote the following macro to help out with that and it seems to work fine.

macro_rules! export {
    ($target:expr, $($types:ty),*) => {
        {
            let target = $target;
            fn _export(target: &mut impl ::std::io::Write) -> ::std::result::Result<(), ::std::io::Error> {
                $(
                    writeln!(target, "export {}\n", <$types as ::ts_rs::TS>::decl())?;
                )*
                Ok(())
            }
            _export(target)
        }
    };
}

fn some_function() {
    let mut target = std::fs::File::create("my-bindings.ts").unwrap();
    let _ = export! {
        &mut target,

        SomeType,
        AnotherType,
        /// ...
    }
}

@NyxCode
Copy link
Collaborator

NyxCode commented Dec 1, 2021

@Heliozoa with #[ts(export)], you get quite a bit more than format!("export {}", MyType::decl()), like imports and formatting.

@NyxCode NyxCode added enhancement New feature or request P-HIGH This will be treated with high priority labels Jul 28, 2022
@escritorio-gustavo escritorio-gustavo added Next Major Release enhancement New feature or request and removed enhancement New feature or request Next Major Release labels Jan 26, 2024
@escritorio-gustavo
Copy link
Contributor

This has been inactive for a very long time and there have not been any PRs aiming at implementing this.
Also, this can somewhat be achieved by adding a feature flag to your own crate and hiding #[derive(TS)] (and even the whole ts-rs crate) behind it, so I think it's safe to close this issue

@escritorio-gustavo escritorio-gustavo closed this as not planned Won't fix, can't repro, duplicate, stale Jan 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request P-HIGH This will be treated with high priority
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants