Add type-checking to unplugin #689
Conversation
|
TypeScript doesn't support incoming source maps and probably never will. We can do the same thing we do in the LSP and remap the diagnostics by using our own source maps like this: https://github.com/DanielXMoore/Civet/blob/main/lsp/source/lib/util.mts#L425 |
|
@STRd6 How can I get source map lines from compiled civet source? |
|
You can pass https://github.com/DanielXMoore/Civet/blob/main/lsp/source/lib/typescript-service.mts#L482 Then it returns a |
|
There's also some utility code for sourcemap mapping. I think in util.civet? |
|
Thanks, yeah this should be working now :) |
|
Could you add some docs to the unplugin README about how this works? |
|
Sure, will do that |
|
Another minor thing: I think |
|
This seems like it should be done, but I'm still failing to run tests. Here's what I tried: integration/unplugin/examples/vitediff --git a/integration/unplugin/examples/vite/src/main.civet b/integration/unplugin/examples/vite/src/main.civet
--- a/integration/unplugin/examples/vite/src/main.civet
+++ b/integration/unplugin/examples/vite/src/main.civet
@@ -1,3 +1,6 @@
{a} from "./module.civet"
-console.log a
+const b = 5
+console.log b()
+
+console.log a + 5
diff --git a/integration/unplugin/examples/vite/vite.config.js b/integration/unplugin/examples/vite/vite.config.js
index 7d9ffb5..bc51b4a 100644
--- a/integration/unplugin/examples/vite/vite.config.js
+++ b/integration/unplugin/examples/vite/vite.config.js
@@ -2,5 +2,5 @@ import civetVitePlugin from '@danielx/civet/vite';
import { defineConfig } from 'vite';
export default defineConfig({
- plugins: [civetVitePlugin({})],
+ plugins: [civetVitePlugin({typecheck: true})],
});Then I run npm install
npm link @danielx/civet
npm run buildThis builds, but doesn't generate any diagnostics. Should it? integration/unplugin/examples/astroJust running: npm install
npm link @danielx/civet
npm run buildcrashes with error: I'm not sure why though; the export looks properly defined. (And I did build Civet, and I checked the link; in particular, |
|
Huh, so my first guess was that it might have something to do with |
|
Do you have a setting where type checking successfully reports errors? (Should I try another example?) I'd be happy to merge this and leave more situations for the future — I'd just like to see it report type errors in at least one setting. 😸 |
|
Vite should be working now 😃 When adding files to |
|
In the Vite example I'm now getting this error: This seems like progress but also not correctly reporting an error. :-) Am I doing something wrong? I did build... |
|
Weird, this seems to working for me. What's the location in |
|
Good news: Vite indeed works for me on Linux. So the error I get (still as above) is Windows-specific. Annoyingly, it does not give a source line number, but with some instrumentation, I find that it's using const program = import_typescript.default.createProgram({
rootNames: [...fsMap.keys()],
options: compilerOptions,
host: host.compilerHost
});I checked Ah yes, the problem is that someone is normalizing filenames to use I think this is the issue we're running into: https://vitejs.dev/guide/api-plugin.html#path-normalization I think one fix could be this: when we call |
|
I added my proposed fix and this now works for me on both Windows and Linux. So I think we're done! |
|
Great find! I remember this is something I had faced issues with when working on Macaron as well, didn't realise we were facing the same issue here. I wonder if this also fixes the Astro issue 🤔 though it seems unrelated to the path formatting |
|
Amazingly, it did fix Astro too! Yay!! By the way, do you think we should add |
|
That's funny, Astro's error messages can be pretty weird sometimes I've seen. I was testing the plugin out again, and it looks like we need to move the
Hmm I don't think so, I like build tools to not do type checking in CLI during dev builds as it slows it down by alot. Maybe we can do it in the examples based on |
|
I'm not getting any errors with the Rollup example using main.civet → dist...
(!) The emitted file "main.civet.d.ts" overwrites a previously emitted file of the same name.
(!) The emitted file "utils\module.civet.d.ts" overwrites a previously emitted file of the same name.
created dist in 361ms
It would be natural to have a |
|
That is because currently the rollup example doesn't have any TS-specific syntax—all of it is inferred. But as soon I add a type declaration to I get this error |
|
Oh, right, I recall you've mentioned that issue before: Rollup needs Somewhat related: Given that we have TypeScript integration in this plugin, I wonder whether it would actually make sense to offer an option of transpiling TS → JS via TypeScript instead of Civet's |
|
That would disable all the other bundler plugins from further processing the code. It could lead to confusing behaviour, so inferring it from |
|
Oops, indeed. Retroactively, I guess I meant "implied by |
Adds type-checking to the unplugin. This currently runs only when
dtsis enabled, though maybe this should be changed.One of the issues with the current implementation though, which i'm not sure can be fixed either, is that the errors point to the location in the compiled ts/js source rather than the original civet source. Ideally tsc should be mapping them using the sourcemaps, but it doesn't do that here.