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

CLI initial concept #304

Open
wants to merge 45 commits into
base: main
Choose a base branch
from
Open

CLI initial concept #304

wants to merge 45 commits into from

Conversation

escritorio-gustavo
Copy link
Collaborator

@escritorio-gustavo escritorio-gustavo commented Apr 9, 2024

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

  • Find a way to only run ts-rs tests through this CLI
    • ❌ Maybe through Ignore ts-rs tests by default #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
  • 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
  • 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 Combine exports into single file #59 (see Allow multiple types to set #[ts(export_to = "...")] to the same file #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)
  • Load configuration from configfile (@NyxCode)
  • (maybe) Combine multiple types in one file? (@NyxCode)
  • (maybe) generate index.ts file, re-exporting types (@NyxCode)

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.

@NyxCode
Copy link
Collaborator

NyxCode commented Apr 10, 2024

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
Copy link
Collaborator

NyxCode commented Apr 10, 2024

  • 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.

@escritorio-gustavo

This comment was marked as resolved.

@escritorio-gustavo
Copy link
Collaborator Author

escritorio-gustavo commented Apr 10, 2024

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

@NyxCode
Copy link
Collaborator

NyxCode commented Apr 10, 2024

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.

@escritorio-gustavo

This comment was marked as off-topic.

@NyxCode
Copy link
Collaborator

NyxCode commented Apr 10, 2024

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

@escritorio-gustavo

This comment was marked as resolved.

@NyxCode
Copy link
Collaborator

NyxCode commented Apr 10, 2024

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.)

@escritorio-gustavo
Copy link
Collaborator Author

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

…t value, this way, it'll obey the user's environment variable if it isn't passed into the CLI
@escritorio-gustavo
Copy link
Collaborator Author

  • Customize representations (e.g number vs bigint)

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

@NyxCode
Copy link
Collaborator

NyxCode commented Apr 10, 2024

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
Copy link
Collaborator

NyxCode commented Apr 10, 2024

#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
Copy link
Collaborator

NyxCode commented Apr 11, 2024

--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" }

@escritorio-gustavo
Copy link
Collaborator Author

--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

@NyxCode
Copy link
Collaborator

NyxCode commented Apr 11, 2024

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

@escritorio-gustavo
Copy link
Collaborator Author

  • 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

@NyxCode
Copy link
Collaborator

NyxCode commented Apr 11, 2024

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.

@escritorio-gustavo
Copy link
Collaborator Author

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

cli/src/main.rs Outdated Show resolved Hide resolved
cli/src/main.rs Outdated Show resolved Hide resolved
cli/src/main.rs Outdated Show resolved Hide resolved

use clap::Parser;
use color_eyre::Result;

mod path;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Had to duplicate ts-rs/src/export/path.rs to make absolute paths

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should we make a ts-rs-core crate to share these types of functions?

@escritorio-gustavo
Copy link
Collaborator Author

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

@NyxCode
Copy link
Collaborator

NyxCode commented Apr 12, 2024

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.

cli/src/cargo.rs Outdated Show resolved Hide resolved
cli/src/cargo.rs Show resolved Hide resolved
@escritorio-gustavo escritorio-gustavo added CLI This issue/PR is related to or requires the implementation of the CLI tool breaking change This PR will change the public API in some way labels Apr 17, 2024
@NyxCode NyxCode linked an issue May 17, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change This PR will change the public API in some way CLI This issue/PR is related to or requires the implementation of the CLI tool
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add index.ts with export for all bindings. How to handle naming conflicts
2 participants