Aleph-Alpha / ts-rs

Generate TypeScript bindings from Rust types
MIT License
1.08k stars 107 forks source link

dedup dependencies #293

Closed kamadorueda closed 5 months ago

kamadorueda commented 6 months ago

Goal

Generate TypeLists containing less duplicates.

Mentions #291 #289

Changes

This is an Internal API change, use a HashSet instead of a Vec, and delay formatting until just when it's needed.

Checklist

NyxCode commented 6 months ago

Nice! Pretty much the same as #292, but I think I like this one more.

NyxCode commented 6 months ago

I explored if generating a custom TypeList impl (which would eliminate the recusion problem completely) would a sensible alternative.

It's definetely possible, but I think it'd require a breaking change to be nice. What I had in mind was

fn dependency_types() -> impl TypeList {
  #[derive(Copy, Clone)]
  struct Deps;
  impl TypeList for Deps {
    fn for_each(self, v: &mut impl TypeVisitor) {
      v.visit::<TypeOfField1>();
      v.visit::<TypeOfField2>();
      Something::dependency_types().for_each(v);
      SomethingElse::generics().for_each(v);
    }
  }
  Deps
}

This breaks with generic parameters though. The naive for_each impl would use them, but it can't. to make that work, Deps would need to be generic as well. Definetely something we could do right now, but it's kinda messy.

If we changed fn dependency_types() -> impl TypeList to fn dependency_types(v: &mut impl TypeVisitor), these problems should go away. That would remove one level of indirection.

So tl;dr: I think we should go ahead with this, and keep this change in mind if someone still runs into the issue or for a breaking 9.0 release some time in the future.

NyxCode commented 5 months ago

Note to myself: Add two tests from other branch.

NyxCode commented 5 months ago

So, I've added some reference counting (probably doesn't have a big performance impact, but it felt like the right thing to do), added the test from the other branch and used a HashSet for types as well.

From my side, this is ready to merge.

NyxCode commented 5 months ago

Thanks @kamadorueda for getting this of the ground!