ts-rs icon indicating copy to clipboard operation
ts-rs copied to clipboard

CLI initial concept

Open escritorio-gustavo opened this issue 1 year ago • 22 comments

Hey @NyxCode! This is a very rough first draft for the CLI tool we've been thinking about. It already handles all of our feature flags and the TS_RS_EXPORT_DIR environment variable

Closes #133

Todo

  • [x] Find a way to only run ts-rs tests through this CLI
    • ❌ Maybe through #253 and adding the -- --ignored flag to the command
    • ✅ Maybe we should gate the generation of tests?
      • ✅ We could add a feature flag to ts-rs that re-enables test generation.
      • ✅ This feature would always be enabled when using this CLI
      • ❌ The entire derive could be turned into a no-op when the feature is off
  • [x] Explore a way of generating an index.ts file to reexport all the generated types
    • ❌ We could, after cargo test finishes, walk the output directory and create an index.ts containing export * from 'file'
    • ✅ During export, generate a metadata file to list all types and where they are exported to
      • ✅ In the CLI, use the metadata file to create the index.ts file
      • ✅ Gate the generation of this file behind a feature (maybe named generate-metadata or something)?
        • Otherwise, if used without the CLI and with the export feature, the tests will generate the metadata file and:
          • This metadata file is never deleted (deletion is only done by the CLI)
          • Each run of cargo test makes the file larger, due to using the append write-mode
        • Users would never manually use this feature, but it would be used by the CLI just like export currently is
  • [x] Explore a way to merge all exports into a single file
    • ✅ Currently being done by reading each file listed in the metadata and copying its data over to index.ts, then deleting the original
    • ❓ While adding this feature, I think I thought of a way to solve #59 (see #316), i.e. allowing multiple export_to attributes with the same file path. I have not written any code for this and have no idea if it will work yet:
      • Implement a modified version of @NyxCode's experiment mentioned in this comment. Again, I haven't quite thought this through, but I'm thinking a HashMap<PathBuf, HashSet<String>>. This will map a file path to a set of type names from std::any::type_name
      • For any type with a file export path:
        • Check if the path is already in the map
          • If it is, check if the type name is in its map entry:
            • If it is, skip this type. It has already been exported
            • If not:
              • Add the type name to the map entry
              • Use OpenOptions with both read and write set to true.
              • Generate the type's imports
              • In the already existing file, seek for a \n\n sequence and write the imports between the two line feeds (This is important because of how the CLI assumes the file is structured)
              • Seek to the end of the file and write the type's export statement
          • If it isn't:
            • Use fs::write_all like in the current behavior to completely override the file.
            • Add an entry to the map with the file path and the type name
  • [ ] Discuss other features of the CLI
    • A configuration file so the user doesn't have to pass the flags every time?
      • How should the flags interact with the config file?
        • Should they be mutually exclusive?
        • Should they coexist with flags overriding the file?
        • Should there be a config flag to pass the path to the file?
      • An init subcommand to help the user create said config file?
    • Whatever else this CLI needs to do that isn't listed here
  • [ ] When we consider it ready, it would be interesting to also make it available through cargo binstall

Feature wishlist

  • [ ] Customize representations (e.g number vs bigint) (@NyxCode) (being worked on in #334)
  • [ ] Load configuration from configfile (@NyxCode) (being worked on in #334)
  • [x] (maybe) Combine multiple types in one file? (@NyxCode) (awaiting review in #316)
  • [x] (maybe) generate index.ts file, re-exporting types (@NyxCode) (See explanation above)

Checklist

  • [ ] I have followed the steps listed in the Contributing guide.
  • [ ] If necessary, I have added documentation related to the changes made.
  • [ ] I have added or updated the tests related to the changes made.

escritorio-gustavo avatar Apr 09 '24 19:04 escritorio-gustavo

Awesome!

For ts-rs, we pretty often do technically breaking releases, but for most users, they are actually drop-in (unless e.g they implement TS manually). For the CLI, we probably want to get that right the first time, so I suggest we carefully plan what we want to support.

I've got a lot of open questions and ideas, but let me start with these two:

  • Should the library be still usable without the CLI? I think it'd be a good idea to keep cargo test working, or maybe require cargo test -- export_bindings, but to keep it usable without CLI.
  • Do we want to enable the *-impl features through the CLI? If we do that, then #[derive(TS)] must be a no-op normally, or code will fail to compile. Even then, if #[derive(TS)] only does something when the CLI is invoked, errors will only show up at that point. As far as I can tell, it'd make more sense for users to enable these features in the Cargo.toml.

NyxCode avatar Apr 10 '24 14:04 NyxCode

  • [ ] Explore a way of generating an index.ts file to reexport all the generated types - We could, after cargo test finishes, walk the output directory and create an index.ts containing export * from 'file'

That'd certainly work. Alternatively, the tests could just output the typescript (or/and metadata) to stdout, and the CLI could maybe combine them, or write the files (and an index.ts) itself.

NyxCode avatar Apr 10 '24 14:04 NyxCode

  • Do we want to enable the *-impl features through the CLI? If we do that, then #[derive(TS)] must be a no-op normally, or code will fail to compile. Even then, if #[derive(TS)] only does something when the CLI is invoked, errors will only show up at that point. As far as I can tell, it'd make more sense for users to enable these features in the Cargo.toml.

You're right, the *-impl features should not be in the cli

  • Should the library be still usable without the CLI? I think it'd be a good idea to keep cargo test working, or maybe require cargo test -- export_bindings, but to keep it usable without CLI.

I think it should, for that we could either merge #253 or add a cargo feature that blocks test generarion, so users would do cargo test --features ts-rs/export or something like that

escritorio-gustavo avatar Apr 10 '24 14:04 escritorio-gustavo

Alternatively, the tests could just output the typescript (or/and metadata) to stdout, and the CLI could maybe combine them, or write the files (and an index.ts) itself.

We'd have to find a way to separate what we want from cargo test's normal output (test result: ok. 1 passed; 0 ...)

the tests could just output the typescript

This would need to be behind a new cargo feature otherwise the lib would not be usable without the CLI

escritorio-gustavo avatar Apr 10 '24 14:04 escritorio-gustavo

add a cargo feature that blocks test generarion, so users would do cargo test --features ts-rs/export or something like that

I like that!

This would need to be behind a new cargo feature otherwise the lib would not be usable without the CLI

Agreed! I'm not sure yet if we need that at all yet, so maybe we should continue to think about features & do that once there's a need for that.

NyxCode avatar Apr 10 '24 14:04 NyxCode

I like that!

Awesome! I'll work on that later today, what do you think the feature should be called?

escritorio-gustavo avatar Apr 10 '24 15:04 escritorio-gustavo

Awesome! I'll work on that later today, what do you think the feature should be called?

Oh, I'm terrible with names. But export seems reasonable :D

NyxCode avatar Apr 10 '24 15:04 NyxCode

Do you think we should gate only the test generation behind the feature or should we make the whole #[derive(TS)] a no-op?

escritorio-gustavo avatar Apr 10 '24 17:04 escritorio-gustavo

Do you think we should gate only the test generation behind the feature or should we make the whole #[derive(TS)] a no-op?

I considered both, but I think making #[derive(TS)] a no-op would be pretty intrusive, and potentially break lots of code (custom setups for exporting, custom TS impls with : TS trait bounds, etc.)

NyxCode avatar Apr 10 '24 17:04 NyxCode

Makes sense! I added an extra check with the cfg!() macro to verify the feature before generating the test

escritorio-gustavo avatar Apr 10 '24 17:04 escritorio-gustavo

  • [ ] Customize representations (e.g number vs bigint)

Maybe we should revisit #94 to have a better idea of how to handle this

escritorio-gustavo avatar Apr 10 '24 19:04 escritorio-gustavo

Getting #49359 would be pretty cool - we could parse the JSON output, and do interesting stuff with it.

$ cargo +nightly t --features ts-rs/export -- -Z unstable-options --format json 2>/dev/null
{ "type": "suite", "event": "started", "test_count": 10 }
{ "type": "test", "event": "started", "name": "export_bindings_complexenum" }
{ "type": "test", "event": "started", "name": "export_bindings_complexstruct" }
{ "type": "test", "event": "started", "name": "export_bindings_gender" }
{ "type": "test", "event": "started", "name": "export_bindings_inlinecomplexenum" }
{ "type": "test", "event": "started", "name": "export_bindings_point" }
{ "type": "test", "event": "started", "name": "export_bindings_role" }
{ "type": "test", "event": "started", "name": "export_bindings_series" }
{ "type": "test", "event": "started", "name": "export_bindings_simpleenum" }
{ "type": "test", "event": "started", "name": "export_bindings_user" }
{ "type": "test", "event": "started", "name": "export_bindings_vehicle" }
{ "type": "test", "name": "export_bindings_complexstruct", "event": "ok" }
{ "type": "test", "name": "export_bindings_gender", "event": "ok" }
{ "type": "test", "name": "export_bindings_point", "event": "ok" }
{ "type": "test", "name": "export_bindings_role", "event": "ok" }
{ "type": "test", "name": "export_bindings_simpleenum", "event": "ok" }
{ "type": "test", "name": "export_bindings_vehicle", "event": "ok" }
{ "type": "test", "name": "export_bindings_series", "event": "ok" }
{ "type": "test", "name": "export_bindings_user", "event": "ok" }
{ "type": "test", "name": "export_bindings_complexenum", "event": "ok" }
{ "type": "test", "name": "export_bindings_inlinecomplexenum", "event": "ok" }
{ "type": "suite", "event": "ok", "passed": 10, "failed": 0, "ignored": 0, "measured": 0, "filtered_out": 0, "exec_time": 0.031168 }
{ "type": "suite", "event": "started", "test_count": 0 }
{ "type": "suite", "event": "ok", "passed": 0, "failed": 0, "ignored": 0, "measured": 0, "filtered_out": 0, "exec_time": 0.0001069 }

NyxCode avatar Apr 10 '24 20:04 NyxCode

#49359 seems kinda close. #50297, on the other hand, seems far off. That's unfortunate, since we could probably do really cool stuff with it, without being too hacky.

NyxCode avatar Apr 10 '24 20:04 NyxCode

--show-output together with --format json seems especially cool. Any println! in the tests turns into

{ "type": "test", "name": "some_test", "event": "ok", "stdout": "hey\n" }

NyxCode avatar Apr 11 '24 00:04 NyxCode

--show-output together with --format json seems especially cool. Any println! in the tests turns into

{ "type": "test", "name": "some_test", "event": "ok", "stdout": "hey\n" }

Now that would make it easier lol

escritorio-gustavo avatar Apr 11 '24 12:04 escritorio-gustavo

With the "generate an index.ts" feature in mind, the CLI needs some way to get a list of all TS types and their respective .ts files. Solutions I can think of are

  • The tests communicate with the CLI by printing to stdout
    • That get's messy without --format json, I'm afraid
      • Maybe it'd be acceptable to require nightly for the CLI until that's stable, idk.
  • The CLI parses the .ts files to figure out which types are in them
    • Lot of duplicated work there
    • Alternative: We generate a comment in the first line of the .ts files, and just parse that.
  • The tests don't generate TS, but json instead, and the CLI does the conversion to TS
  • The tests generate an additional metadata file, which the CLI will parse

NyxCode avatar Apr 11 '24 13:04 NyxCode

  • The tests generate an additional metadata file, which the CLI will parse

I think this might be the best way to go. Use the mutex file lock to append to a metadata file all the types' names anc paths, then parse the metadata file to generate the index.ts file

escritorio-gustavo avatar Apr 11 '24 17:04 escritorio-gustavo

I think this might be the best way to go. Use the mutex file lock to append to a metadata file all the types' names anc paths, then parse the metadata file to generate the index.ts file

I suspect that it might be simpler to write one metadata file per test, but i'd be happy to be convinced otherwise.

NyxCode avatar Apr 11 '24 17:04 NyxCode

I suspect that it might be simpler to write one metadata file per test, but i'd be happy to be convinced otherwise.

I agree that this would probably be simpler, but we'd have to walk the output searching for these metadata files, so we could just walk the diretory reading the ts files instead

escritorio-gustavo avatar Apr 11 '24 19:04 escritorio-gustavo

This all kinda makes me curious - How would it look like if we tried to do that without a CLI? Gotta play with that during the weekend!

NyxCode avatar Apr 12 '24 20:04 NyxCode

This all kinda makes me curious - How would it look like if we tried to do that without a CLI? Gotta play with that during the weekend!

My guess is that it'd be pretty much impossible, at least the way we've implemented it so far, as it heavily relies on doing stuff after cargo test has finished

escritorio-gustavo avatar Apr 12 '24 20:04 escritorio-gustavo

My guess is that it'd be pretty much impossible, at least the way we've implemented it so far, as it heavily relies on doing stuff after cargo test has finished

I suspect that's true. For something like example/, I think we could make it work with a static holding the current state, but as soon as there are multiple test executables, like in the ts-rs test suite, stuff get's very complicated.

NyxCode avatar Apr 12 '24 20:04 NyxCode