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

[Fix] Disable type checking for non-typescript projects by default #4495

Closed

Conversation

Theo-Steiner
Copy link
Contributor

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Ever since #4118 .js files are type checked for newly created projects despite ? Use TypeScript? › No being selected during the create-svelte wizard.
This has been described in #4428 and also causes unwanted error messages as #4427 mentions.

I think type checking for non-typescript projects really should be disabled by default as quite a few newcomers on the discord (today, yesterday) are confused by type errors popping up, even though they didn't select typescript in the CLI.
This behavior seems to surprise even experienced developers: just yesterday Vercel's Lee Robinson stumbled over this in his stream on SvelteKit.
The red squiggly lines on a newly scaffolded out default app also are a bad look IMO.

Maybe we could put a section into the docs for how to re-enable this behavior though? Since it definitely can be helpful when desired.

Closes #4428, closes #4427.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpx changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

@changeset-bot
Copy link

changeset-bot bot commented Apr 2, 2022

🦋 Changeset detected

Latest commit: cee9253

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
create-svelte Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

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

Agree. AFAIK this was changed by accident when the generated tsconfig was introduced, before that the starter template didn't have type checking for JS.
I'm thinking about whether or not we should turn this around and instead have the generated tsconfig contain less (no checkJs for example) and add that to the tsconfig instead when generating a TS project.

@Rich-Harris
Copy link
Member

I almost wonder if we need to have two separate prompts when setting up a project, where if you don't opt in to TypeScript-the-language you can still opt in to typechecking:

? Use TypeScript? › Yes
? Use TypeScript? › No
? Add typechecking for JavaScript? › No / Yes

Personally I would choose No and Yes, but I don't know if I'm in a sufficiently small minority that that would be annoying for most non-TypeScript users.

@gtm-nayan
Copy link
Contributor

gtm-nayan commented Apr 5, 2022

Personally I would choose No and Yes

Users who choose to do that with the todos demo would still get the type errors I mentioned in #4427. I've been suggesting people to add a form.d.ts with the action's types when they complain about this over on Discord. I tried to conditionally include the file but I'm failing to understand the +- thing.

that would be annoying for most non-TypeScript users

That shouldn't be a problem with dynamic prompts, could also consider making it a single selection between, Typescript, Vanilla JS, and JS with some type checking.

@Theo-Steiner
Copy link
Contributor Author

Personally I would choose No and Yes, but I don't know if I'm in a sufficiently small minority that that would be annoying for most non-TypeScript users.

I would think that JSdoc users are enough of a minority to justify not introducing an additional prompt for enabling JSdoc. Especially given that the overhead for manually turning it on is literally just flipping one boolean.

Also the additional configuration necessary mentioned by @gtm-nayan could mean adding an additional maintenance burden to create-svelte.

@mrkishi
Copy link
Member

mrkishi commented Apr 5, 2022

checkJs isn't aimed exclusively at JSDoc users, though. Personally I unconditionally enable checkJs on all projects I browse as I consider the type-checking analysis useful—even if at times I come across misplaced errors.

gtm-nayan's suggestion of a single selection between TS, JS and JS with type-checking has my vote.

@Theo-Steiner
Copy link
Contributor Author

Theo-Steiner commented Apr 5, 2022

I tried my hand at modifying create-svelte to allow for the three way selection favored by gtm-nayan and mrkishi.
Would love to hear your feedback!
And I also got stuck at the same problem as gtm-nayan, not knowing how to type the enhance action in the checkJs case.

Edit: how come pnpm lint is fine locally but not in the ci?

@gtm-nayan
Copy link
Contributor

gtm-nayan commented Apr 10, 2022

Something like this maybe?

Still stuck on the ts -> dts part tho, @dummdidumm is there a straight forward way of extracting declarations from a .ts file without the boilerplate of ts.createCompilerHost() yada yada?

@gtm-nayan
Copy link
Contributor

I see two possible solutions for the type declarations that wouldn't require a significant overhaul,

either we write the type declarations manually and put it inside the appropriate folder under shared/, possible problem is the inconvenience of having to keep them in sync (until the form component from #3533 becomes a thing), this way we can include declaration files when the user picks typechecking without typescript and exclude them otherwise

or we include the declarations regardless of checkJs when typescript is disabled with the benefit of not having to sync them manually

I prefer the latter because I think it's better to have declarations if someone decides to flip the checkJs switch afterwards and for autocomplete. I've made a PR over to Theo's branch going with the second one.

Thoughts?

@Rich-Harris
Copy link
Member

Another idea is to add JSDoc annotations to the source code, and only strip them out if the user chooses typescript. I'd argue this is better than having .d.ts files. I opened a new PR for that: #4621

@Rich-Harris
Copy link
Member

Going to close this in favour of #4621 — thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants