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 for type checking #61

Closed
lorefnon opened this issue Dec 17, 2022 · 10 comments · Fixed by #1023
Closed

CLI for type checking #61

lorefnon opened this issue Dec 17, 2022 · 10 comments · Fixed by #1023
Labels
cli Command-line interface issues enhancement New feature or request typescript TypeScript compatibility or extensions

Comments

@lorefnon
Copy link
Contributor

Vue has a cli utility called vue-tsc, which can be used for type-checking in CI env etc. It would be good to have something similar for civet.

@edemaine
Copy link
Collaborator

Agreed. I was imagining a civetsc CLI tool (or something like that) that runs TypeScript on the transpiled files, following tsconfig.json (or maybe even tsconfig.civet?).

Perhaps it should be a second bin for the main Civet package. Or perhaps it should be its own NPM package, given that it would have a typescript peer dependency?

@lorefnon
Copy link
Contributor Author

runs TypeScript on the transpiled files

That may not be a great idea. Ideally we'd non-interactively do what the lsp client already does so that any errors etc. have correct source line numbers.

@STRd6 Are you open to bin/civettsc (or maybe just civetc) ? I think keeping this outside may be a maintenance burden because I think tsserver integration is still early and keeping the two in sync may need more work.

@edemaine
Copy link
Collaborator

I agree that this should be part of this repo (but maybe separate NPM package), and wyen I said "run TypeScript" I meant the same thing as the LSP (not running tsc).

@STRd6
Copy link
Contributor

STRd6 commented Dec 18, 2022

Having an additional civetc build product in bin makes sense. I also agree that making use of the same TSServer wrapping is important so that the editor error messages match the cli error messages exactly. I'm in support of anyone who wants to work towards this.

@lorefnon
Copy link
Contributor Author

Ok cool. Looks like everyone is on the same page then.

I can take this up mid next week.

@STRd6
Copy link
Contributor

STRd6 commented Dec 18, 2022

Feel free to refactor out the TSServer wrapping from lsp into a shared lib if it makes things easier.

@edemaine edemaine added enhancement New feature or request cli Command-line interface issues labels Jan 4, 2023
@edemaine edemaine added the typescript TypeScript compatibility or extensions label Mar 26, 2023
@edemaine
Copy link
Collaborator

An update on this: if you use Vite, Rollup, ESBuild, or Webpack to build your Civet code, our new unplugin for these systems now has a typecheck: true option that will do typechecking. So we now have a reasonable way to do type checking in CLI!

@edemaine
Copy link
Collaborator

edemaine commented Feb 1, 2024

We should try to extract the typecheck: true feature from the unplugin into a module that the CLI could directly call. This would still require installing typescript locally (or globally?), but would make it easy to integrate into the existing CLI with a command-line option like civet --typecheck or civet -t.

@danielbayley
Copy link
Contributor

Surely the bin should be called tscivet… or tscvt?

@edemaine
Copy link
Collaborator

edemaine commented Feb 1, 2024

I guess it's a tradeoff on whether to have just one bin or multiple. I don't have a strong feeling, but most of the code would be shared with the CLI so I think allowing for civet -t at least makes sense. We could also add an alias of tscivet or civetsc if we want.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli Command-line interface issues enhancement New feature or request typescript TypeScript compatibility or extensions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants