breaking: remove param files in folder in favor of params.js/ts file - #16189
Conversation
basically implements #10407 (comment): - new `parse` function can be exported from `params/someMatcher.js` - can be either alongside `match` or standalone - if it throws it counts as invalid and the route will not be matched closes #10407
match can now also be a standard schema. If it parses you get the transformed value. If it throws it counts as invalid and the route will not be matched. closes #10407
…`'~standard'` property, so callable Standard Schemas (e.g. ArkType) are misclassified as plain predicate functions, breaking validation and transformation.
This commit fixes the issue reported at packages/kit/src/utils/routing.js:139
## Bug
`ParamMatcher` is typed as `((param: string) => boolean) | StandardSchemaV1<string, any>`. Several headline Standard Schema implementations expose their schema as a **callable function that also carries a `'~standard'` property**. ArkType is the canonical example:
```js
const t = type('string.numeric.parse');
t('42'); // → 42 (parsed value, or an ArkErrors object on failure)
t['~standard'].validate('42'); // also available
```
In the original `run_matcher`, the `typeof matcher === 'function'` branch comes first:
```js
if (typeof matcher === 'function') {
return matcher(value) ? { success: true, value } : { success: false };
}
if ('~standard' in matcher) { ... }
```
Because an ArkType schema is a function, it never reaches the `'~standard'` branch. Instead `matcher(value)` is called and its return value treated as a boolean:
- **Invalid input**: ArkType returns a truthy `ArkErrors` object → treated as a *successful* match, and the raw, untransformed string is stored as the param value. The route matches when it should not.
- **Valid input that transforms to a falsy value** (e.g. `0`, empty string): the truthiness check fails → the route is incorrectly rejected.
So both validation and transformation are broken for callable Standard Schemas.
## Fix
Reorder the checks so `'~standard'` is tested first. `'~standard' in matcher` works on functions too (functions are objects), so a callable Standard Schema is now routed through the proper `validate` path. Plain predicate functions lack the `'~standard'` property and correctly fall through to the function branch.
```js
function run_matcher(matcher, value) {
if ('~standard' in matcher) {
const result = matcher['~standard'].validate(value);
if (result instanceof Promise || result.issues) {
return { success: false };
}
return { success: true, value: result.value };
}
if (typeof matcher === 'function') {
return matcher(value) ? { success: true, value } : { success: false };
}
return { success: false };
}
```
Co-authored-by: Vercel <vercel[bot]@users.noreply.github.com>
Co-authored-by: dummdidumm <sholthausen@web.de>
Instead of having to declare one param matcher per file, you now declare all of them within one file. A `defineParams` function both helps with declaring them in a type-safe way and also normalizes the property values: You can either parse a standard schema object, or a function that either throws or returns the parsed value (if you don't want to bring in a schema library). closes #10407 Kudos to @phi-bre for suggesting this in #16148 (comment)
🦋 Changeset detectedLatest commit: 799451d 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 |
…cing the invalid path `src/params/.js` instead of `src/params.js`.
This commit fixes the issue reported at packages/kit/src/core/sync/write_types/index.js:605
## Bug
In `generate_params_type` (`write_types/index.js:606`) and the identical logic in `write_non_ambient.js:101`, the fallback used when `resolve_entry` returns `null` was:
```js
path.join(config.kit.files.params.replace(/.(js|ts)$/, ''), '.js');
```
`config.kit.files.params` defaults to `path.join(files.src, 'params')` → `src/params` (no extension). The `.replace(/.(js|ts)$/, '')` is a no-op there, and `path.join('src/params', '.js')` inserts a path separator, yielding `src/params/.js`. Even when the config value has an extension (e.g. `src/params.ts`), the replace strips it to `src/params` and `path.join` again yields `src/params/.js`.
Verified with Node:
```
$ node -e "console.log(require('path').join('src/params', '.js'))"
src/params/.js
```
### Trigger / impact
When a route references a matcher but `resolve_entry(config.kit.files.params)` returns `null` (params file not found at resolution time), the generated `$types`/app-types contain an import from the broken path `src/params/.js` instead of `src/params.js`, producing invalid type imports.
## Fix
Replaced `path.join(base, '.js')` with string concatenation `base + '.js'` in both locations:
```js
config.kit.files.params.replace(/.(js|ts)$/, '') + '.js';
```
Verified output is now `src/params.js` for both `src/params` and `src/params.ts` inputs.
Co-authored-by: Vercel <vercel[bot]@users.noreply.github.com>
Co-authored-by: dummdidumm <sholthausen@web.de>
|
Hello, Two remarks.
For example if I have this params : export const params = defineParams({
details: v.picklist(['min', 'standard', 'max']);
});I think this should be typed accordingly : resolve('/info/[details=details]', { details: 'min' }); // OK
resolve('/info/[details=details]', { details: 'minimum' }); // Type Error
For example, if I have bigint as uid, and I want to use it as hexadecimal on the URL : export const params = defineParams({
uid: {
// parse the uid as a hexa BigInt :
parse: v.pipe(v.string(),v.transform((value) => BigInt(`0x${value}`)),
// format the BigInt in hexa
format: (value) => value.toString(16)
}
}); |
params.js/ts fileparams.js/ts file
Co-authored-by: Conduitry <git@chor.date>
|
Food for thought, Though prior art is mixed:
|
|
this is better than what we currently got. one question though, will this feature be backported to kit v2 behind an experimental flag? docs dont mention anything about that |
|
no there are no plans to backport this at the moment |
|
Install the latest version of pnpm add https://pkg.svelte.dev/@sveltejs/kit/c/799451de227f726e4bc967125c2b980e693c6f90Open in |
that wouldn't really make sense. |
…try-catch on server.
There was a problem hiding this comment.
Additional Suggestion:
A throwing param matcher breaks client-side navigation (silent no-op / unhandled promise rejection) even though the server handles the same throw gracefully, creating a server/client asymmetry introduced by commit f5479de.
|
Vercel is right, but I don't see a good way how we can sensibly handle this. We could fall back to a full page reload to get the better error message eventually that way. // in function navigate(...)
try {
intent ??= await get_navigation_intent(url, false);
} catch {
// Likely a matcher threw unexpectedly. Fall back to the server to get a proper error page.
return await native_navigation(url, replace_state);
}Anything more would mean polluting the whole |
Rich-Harris
left a comment
There was a problem hiding this comment.
couple of suggestions but LGTM
Co-authored-by: Rich Harris <hello@rich-harris.dev>
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to version-3, this PR will be updated.⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ `version-3` is currently in **pre mode** so this branch has prereleases rather than normal releases. If you want to exit prereleases, run `changeset pre exit` on `version-3`.⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ # Releases ## @sveltejs/kit@3.0.0-next.7 ### Major Changes - breaking: change `form.error` type from `any` to `App.Error | undefined` ([#16245](#16245)) - feat: allow adapters to provide additional Vite plugins ([#16206](#16206)) - breaking: move tracing out of the experimental namespace and remove the instrumentation flag ([#16260](#16260)) - breaking: remove `$app/stores` ([#15499](#15499)) - breaking: disallow `*.remote.ts/js` files unless `experimental.remoteFunctions` is enabled ([#16247](#16247)) - breaking: remove param files in folder in favor of `params.js/ts` file ([#16189](#16189)) ### Patch Changes - fix: defer `query.refresh()` in server commands until after the command body completes ([#16225](#16225)) - fix: resolve service worker and `tsconfig.json` based on Vite `root` setting ([#16229](#16229)) - fix: populate `$app/env/*` dynamic variables in contexts that don't run the dev server, such as `vite-node` ([#16223](#16223)) - fix: no longer throw "An impossible situation occurred" when a server-only module is imported by both server and client code ([#16257](#16257)) - fix: set `define` values on `globalThis` when running Vitest ([#16246](#16246)) - fix: serve `.ico` files with `image/x-icon` Content-Type ([#16234](#16234)) - fix: make `paths.origin` type looser ([#16215](#16215)) - fix: avoid client build warning about externalising `node:async_hooks` ([#16244](#16244)) - fix: allow reserved words (e.g. `delete`, `class`) as remote function export names ([#16264](#16264)) ## @sveltejs/adapter-netlify@7.0.0-next.2 ### Patch Changes - fix: ensure types for `platform.context` work ([#16255](#16255)) - Updated dependencies [[`ba78a0b`](ba78a0b), [`a248c9b`](a248c9b), [`7596981`](7596981), [`be72ed9`](be72ed9), [`a5bd7e2`](a5bd7e2), [`dec671f`](dec671f), [`82f3867`](82f3867), [`97eb324`](97eb324), [`e2a9ac0`](e2a9ac0), [`309bfb4`](309bfb4), [`522a86b`](522a86b), [`0ff547f`](0ff547f), [`3d4ff91`](3d4ff91), [`c925f2a`](c925f2a), [`7daf445`](7daf445)]: - @sveltejs/kit@3.0.0-next.7 Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…#16403) `normalize_param_definition` checks for a function before checking for `~standard`, but standard schemas can themselves be callable. ArkType's `type()` returns one, so it gets wrapped as a plain function matcher and its own validation never runs. The wrapper then treats whatever the call returns as the parsed value, and ArkType returns an `ArkErrors` instance rather than throwing: ```js const matcher = normalize_param_definition(type('string.numeric')); matcher['~standard'].validate('not-a-number'); // { value: ArkErrors } so the route matches, with an ArkErrors instance as the param ``` Checking `~standard` first fixes this, verified against arktype 2.x (invalid values now produce `issues`, so the route no longer matches). It also matches `ParamEntry` in `public.d.ts`, which has resolved callable schemas to the schema branch since #16189. Same discriminator fix as #16402. The new test fails on `version-3`. --- ### Please don't delete this checklist! Before submitting the PR, please make sure you do the following: - [x] 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 - [x] This message body should clearly illustrate what problems it solves. - [x] Ideally, include a test that fails without this PR but passes with it. ### Tests - [x] Run the tests with `pnpm test` and lint the project with `pnpm lint` and `pnpm check` ### Changesets - [x] 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 - [x] Please ensure that 'Allow edits from maintainers' is checked. PRs without this option may be closed.
Instead of having to declare one param matcher per file, you now declare all of them within one file. A
defineParamsfunction both helps with declaring them in a type-safe way and also normalizes the property values: You can either parse a standard schema object, or a function that either throws or returns the parsed value (if you don't want to bring in a schema library).closes #10407
Kudos to @phi-bre for suggesting this in #16148 (comment)