fix: add a default "include": [] array to generated tsconfig.json, and warn if not populated by user config - #16593
fix: add a default "include": [] array to generated tsconfig.json, and warn if not populated by user config#16593Rich-Harris wants to merge 2 commits into
"include": [] array to generated tsconfig.json, and warn if not populated by user config#16593Conversation
…and warn if not populated by user config
|
Install the latest version of pnpm add https://pkg.svelte.dev/@sveltejs/kit/c/f38741024077e9e5342d5dda8f502978121c8018Open in |
🦋 Changeset detectedLatest commit: f387410 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
| const resolved = ts.parseJsonConfigFileContent(user_config.options, ts.sys, dir); | ||
| const warnings = validate_resolved_config(resolved.options, config.compilerOptions); | ||
|
|
||
| if (resolved.raw.include?.length === 0) { |
There was a problem hiding this comment.
If it's completely missing, this would evaluate to undefined, which doesn't equal 0 - right? Should we just have !resolved.raw.include?.length?
There was a problem hiding this comment.
Or is this going to be inheriting from the generated one, which has []? In which case, why the ?.? Is it just because the types can't know that?
There was a problem hiding this comment.
it's intentional — if include is unspecified, everything inside the directory containing the tsconfig.json is included. if it's specified-but-empty, as is the case for a user config that extends $app/tsconfig, nothing will be included. i guess this could be clearer
| if (resolved.raw.include?.length === 0) { | |
| // `$app/tsconfig` specifies `include: []` — if the user config doesn't override this, | |
| // nothing will be included. `$app/tsconfig/service-worker` doesn't specify `include`, | |
| // which is fine — everything in `src/service-worker` is included by default. | |
| // this warns on `[]`, but ignores `undefined` | |
| if (resolved.raw.include?.length === 0) { |
| const warnings = validate_resolved_config(resolved.options, config.compilerOptions); | ||
|
|
||
| if (resolved.raw.include?.length === 0) { | ||
| warnings.push(`Missing "include" array`); |
There was a problem hiding this comment.
This message is not providing enough context and needs actionable advice.
And why do we not do src, test ourselves?
There was a problem hiding this comment.
Might be because the user should specify those themselves in the root tsconfig.json. If we did it for them, the moment they add a new entry to their root include, it overrides what we've written. Better that they specify everything themselves and adjust the one include array
There was a problem hiding this comment.
Exactly — it's a lot more visible if it's in their own config. Ideally the CLI would put src and test and *.ts in automatically.
What about this?
| warnings.push(`Missing "include" array`); | |
| warnings.push(`Missing "include" array. Consider adding \`"include": ["src", "test", "*"]\``); |
There was a problem hiding this comment.
Or more verbosely:
| warnings.push(`Missing "include" array`); | |
| warnings.push(`Missing "include" array, which means nothing will be typechecked. Consider adding \`"include": ["src", "test", "*"]\``); |
(Trying to avoid making things too verbose, because the text wraps in an ugly awkward way if it takes multiple lines)
There was a problem hiding this comment.
The first is better because technically everything will get type checked when it's missing. Like if you did a build it would type check your build output files. Maybe we can swap the "*" with a "your-directory" or "your-file"
4 participants
closes #16581
Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm testand lint the project withpnpm lintandpnpm checkChangesets
pnpm changesetand following the prompts. Changesets that add features should beminorand those that fix bugs should bepatch. Please prefix changeset messages withfeat:,fix:, orchore:.Edits