fix: allow optional remote form schema fields under {exactOptionalPropertyTypes: true}

[echocrow]

closes #15568

Naively allows undefined type in form input values to satisfy type checks when exactOptionalPropertyTypes is enabled.

This PR also adds previously-failing tests. To actually test for this, the TS compiler option exactOptionalPropertyTypes: true must be set.
Seems that the current testing strategy for the test/types file was to run tsc. Unfortunately this throws all sorts of errors when with exactOptionalPropertyTypes for the new file, because tsc doesn't limit that compiler option to just that one test file, but also to all other included/imported files, too. However, kit core was not written with that compiler option in mind, resulting in a lot of false negatives.
To bypass that issue and still add tests, I opted to move the type check automation from tsc to vitest typecheck. Technically Vitest's typecheck is still experimental, but we've been using it successfully for some time.

---

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.

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 pnpm changeset and following the prompts. Changesets that add features should be minor and those that fix bugs should be patch. Please prefix changeset messages with feat:, fix:, or chore:.

Edits

• Please ensure that 'Allow edits from maintainers' is checked. PRs without this option may be closed.

[changeset-bot]

🦋 Changeset detected

Latest commit: 6d6a58e

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

This PR includes changesets to release 1 package

Name	Type
@sveltejs/kit	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

[svelte-docs-bot]

Preview: https://svelte-dev-git-preview-kit-15866-svelte.vercel.app/

[echocrow]

I noticed that kit's main Vitest config was given a custom name along with this note:

// this file needs a custom name so that the numerous test subprojects don't all pick it up

does the requirement apply here too? if so, could name it e.g. kit-types.vitest.config.js. either way, may make sense to co-locate it next to kit.vitest.config.js(?)

alternatively, we could probably also move this config into kit.vitest.config.js. by default, that would also move type checks out of the check command and become part of test:unit instead. could maybe avoid that by filtering tests, but separate configs are probably preferred, if type checks and unit tests should be associated with different scripts

[teemingc]

I'd recommend not using Vitest for this at all. Our pnpm check command simply navigates to the folder with the tsconfig.json file and runs tsc to see if it spits out type compilation errors. You could extend the npm script to navigate into another folder with a different tsconfig.json

[echocrow]

you could extend the npm script to navigate into another folder with a different tsconfig.json

@teemingc that's exactly what I'd tried at first!

the check command got a little bloated, but that was the lesser issue. the bigger issue with that approach is that it spits out a wall of additional compile errors for the added exactOptionalPropertyTypes tests, because tsc would apply {exactOptionalPropertyTypes: true} to kit source code imported in the test files too, essentially type checking parts of kit src with that compiler flag. but that source code was not written with that flag in mind, hence a spam of false positives.

as far as i can tell, vitest typecheck is effectively running type compile checks via tsc (unless another checker was explicitly configured). by default, that would cause the same false-positive type errors as just running tsc directly. however, vitest implemented a convenient ignoreSourceErrors flag (enabled in this PR) that only flags tsc errors that were reported in the test code, but ones that originated from the source code

also noteworthy: i did not remove the main tsc for type checking kit's source code! i've only moved the type checks for test/types to vitest typecheck

so the options i'm seeing:

• (a) vitest typecheck with ignoreSourceErrors to ignore irrelevant source code
  • that's what this PR is doing
• (b) yolo and not add type checks for the exactOptionalPropertyTypes case
• (c) keep tsc, but somehow ignore source code errors (aka what vitest implemented)?
• (d) alter a bunch of types in kit source with exactOptionalPropertyTypes in mind
  • for the most part, migrating a bunch of {myOptionalKey? MyType} tpo {myOptionalKey? MyType | undefined}. this is a niche opinionated choice, and probably not desired
  • this would need to be done either for all types, or "just the types that coincidentally get checked by tsc from remote.test.ts"

(a) seemed like the least impactful choice while still adding a previously-breaking test

[teemingc]

Thanks for explaining that. I wonder if this means that users who run tsc will run into this issue too, effectively making this a band aid rather than a proper fix. Maybe supporting exactOptionalPropertyTypes requires a bigger change to the Kit codebase

[echocrow]

(edit: typed this before i saw your new post above, following up on that in another post)

update: took a 2nd stab at this via tsc directly

because kit src code is written in JS, there is an option to replicate vitest typecheck's ignoreSourceErrors with these compilerOptions for the added test/type's config

"checkJs": false,
"skipLibCheck": true

this then effectively ignores tsc errors in kit src code when running tsc on e.g. the newly added exactOptionalPropertyTypes/remote.test.ts

this is more of a coincidental property, rather than vitests explicit and more descriptive ignoreSourceErrors

@teemingc with all that in mind, do you still prefer keeping the previous tsc approach, or you cool with the suggested move tovitest typecheck for just kit's test/types? (tsc for kit src is kept as-is and vitest still uses tsc for typecheck under the hood)

[teemingc]

I think we can probably merge this in without the test since it's more setup than it's worth at the moment. I'll let another maintainer who's more familiar with remote functions look at this

[dummdidumm]

I agree, let's revert the vitest stuff, if there's no "optional property" test then add one (but without the tsconfig adjustment) so we can at least check that the change does not break the current configs.

[echocrow]

@dummdidumm cheers for hopping on!

if there's no "optional property" test then add one

there is; see this diff

(but without the tsconfig adjustment)

how do you propose we should go about this?

from what i've seen and tested, the tsconfig for this edge case is needed to get a failing test before the change. without it, the test would have not failed, and thus doesnt add value(?). per linked issue, the type error does not occur without exactOptionalPropertyTypes; but with that compiler option, the example in the docs will throw a type error (see repro in #15568)

summarizing from earlier comments, options im seeing (from low to high effort):

• (a) no change, wont fix, boo me
• (b) just ship the | undefined type addition to RemoteFormInput, hope it wont break in the future
• (c) add type addition & a test case (with dedicated tsconfig with {exactOptionalPropertyTypes: true}); test with tsc (needs some more tsconfig juggling to avoid false negatives, see this comment)
• (d) add type addition & a test case (with dedicated tsconfig with {exactOptionalPropertyTypes: true}); test with vitest typecheck (uses tsc under the hood; with ignoreSourceErrors to avoid false positives; see this comment for details) - current state of this PR
• (e) fully adopt exactOptionalPropertyTypes in kit, fix new kit src type errors as needed (mostly adding | undefined to a bunch of optional properties), add the test case (this time without a dedicated tsconfig; base tsconfig now covers exactOptionalPropertyTypes)

not sure if there is an option between (b) and (c) as you're asking? but maybe i'm missing something

personally leaning (d), but will revert if vitest typecheck is not welcome for checking types. just need to understand how you'd like me to handle the type test for this compilerOption-specific case

[echocrow]

just saw that #15825 got merged (🎉!) which addresses another quirk in projects with exactOptionalPropertyTypes. looks like it tests the behavior by making exactOptionalPropertyTypes part of the tsconfig already in place for other related tests (in packages/kit/src/core/sync/write_types/test/app-types/tsconfig.json)

if that's an acceptable change, i can roll with the same approach here and drop the newly added tsconfig.
TBD how to go about the type errors in source code under exactOptionalPropertyTypes. i suspect the overridden paths for @sveltejs/kit and $app/types is how that app-types test bypasses this issue; fingers crossed the same idea works and is accepted here

[echocrow]

latest version is up! @dummdidumm

back to a single tsconfig, and the tsc approach for type checking as requested

inconveniences along the way:

• I still had to sneak in a skipLibCheck
  • couldn't figure out another workaround to silence a tsc error in a vite dependency.
  • it seems that the tests in write_types use the same workaround; search for --skipLibCheck.
• couldn't find a workaround for some source type errors under exactOptionalPropertyTypes
  • I believe write_types doesn't run into these, because, unlike these tests here, it does not import from $app/server
  • I opted to partially patch the types were necessary for the types to pass. that did, unfortunately, result in some awkward type casts though.

someone else should review it