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

svelte-check error printed to console #1988

Closed
benmccann opened this issue Jul 22, 2021 · 10 comments · Fixed by #2284
Closed

svelte-check error printed to console #1988

benmccann opened this issue Jul 22, 2021 · 10 comments · Fixed by #2284
Labels
bug Something isn't working p2-nice-to-have SvelteKit cannot be used by a small number of people, quality of life improvements, etc.

Comments

@benmccann
Copy link
Member

benmccann commented Jul 22, 2021

Describe the bug

svelte-check seems to be looking for a svelte.config.js file perhaps and then spitting out an error when it can't find it?

Reproduction

cd packages/kit
pnpm check

Logs

│ Error while loading config
│ TypeError: Cannot read property 'compilerOptions' of undefined

System Info

kit `master`

Severity

annoyance

Additional Information

No response

@benmccann benmccann added the bug Something isn't working label Jul 22, 2021
@benmccann
Copy link
Member Author

benmccann commented Jul 23, 2021

Hmm. It seems to be choking on the empty file: packages/kit/src/core/config/test/fixtures/export-missing/svelte.config.js

@benmccann
Copy link
Member Author

benmccann commented Jul 23, 2021

@dummdidumm I might need your help with this one. I can't figure out how to get svelte-check to ignore src/core/config/test/fixtures/export-missing. I've tried adding it to the --ignore flag and adding it to exclude in tsconfig.json

I can't change the file to make it a valid one because the test is testing for an invalid file

@jasonlyu123
Copy link
Member

Since he's on vacation, is there anything I can help with on the language-tools side? This error it's logged with console. error. And it would fall back to using the default config. So it probably it's just confusing but won't fail the test.

The ignore flag currently only affects the source file diagnostic. And the config loading is run before all the diagnostics without considering the ignored path. I think it makes sense to not search for config files in ignored paths. But since this is more of "annoying" behaviour. I think we can have more discussion and considering before fix this.

@benmccann
Copy link
Member Author

Oh, right. I forgot he was away

It's just annoying as you said and so not very high priority

It's not printed when running the test (or if it is it's lost in the other output), but rather when running pnpm check

Considering --ignore when doing config loading sounds reasonable to me and is what I'd expect

@benmccann benmccann added the p2-nice-to-have SvelteKit cannot be used by a small number of people, quality of life improvements, etc. label Aug 4, 2021
@dummdidumm
Copy link
Member

Can't we add an empty export default {} to these files instead? That would hopefully silence the error as well without having to think about edge cases like "--ignore ignores folder X, but something inside that folder is imported from a non-ignored file".

@ignatiusmb
Copy link
Member

ignatiusmb commented Aug 16, 2021

Narrowing down the config files, I found the suspecting error to come from https://github.com/sveltejs/kit/blob/master/packages/kit/src/core/config/test/fixtures/export-missing/svelte.config.js

Edit: It was already pointed out above, but I meant to say that it was deliberately made empty, so adding an empty export would probably defeat the purpose. Maybe we could just omit this one test or ignore the config folder just like the make_package tests.

As for the current ignored paths, that might be a good idea, we could add those empty exports and adjust the package.json as well to have type module.

@benmccann
Copy link
Member Author

I just upgraded svelte-check so that it now prints out more details about the error and it's clear what's happening

Still, I don't know how to remove this warning since the file is purposefully malformed for testing purposes.

I suppose if config loading got moved to its own package we wouldn't have to run svelte-check on that package since it presumably wouldn't have any .svelte files in it

@dummdidumm
Copy link
Member

Some technical background why this all is so hard to tackle:

  • svelte.config.js needs to be loaded in an asynchronous way
  • language-tools, specifically TypeScript, needs synchronous code paths
  • in order to invoke svelte2tsx correctly, the config needs to be known (mainly because of the default language option in svelte-preprocess - another point why I'd like to get rid of it)
  • the two points above contradict each other. Moreoever, TypeScript will throw an error if you tell it "this file is now TS instead of JS". Therefore, upon seeing the first Svelte file, the file path is walked up to find a svelte.config.js, but the path is also walked down to find possibly nested configs, which we then don't need to load afterwards --> this behavior likely produces the warning, even if you --ignore the path
  • Also related: --ignore does nothing when you reference a tsconfig.json, because the referenced files are determined by that then instead (also see [svelte-check] --ignore flag gets overwritten by --tsconfig language-tools#1074).

@ignatiusmb
Copy link
Member

I wonder if we should just remove that specific test

@benmccann
Copy link
Member Author

I wouldn't mind that as a solution. It doesn't seem like the most important thing to have

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working p2-nice-to-have SvelteKit cannot be used by a small number of people, quality of life improvements, etc.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants