Generate declaration files #9895
Conversation
|
There was a problem hiding this comment.
Using the JSdoc output without further cleanup looks like this on hover:
which is not very pleasing an the @template is straight up confusing for someone not familiar with it. This function makes it nicer:
function adjust(input) {
// Extract the import paths and types
const import_regex = /import\(("|')(.+?)("|')\)\.(\w+)/g;
let import_match;
const import_map = new Map();
while ((import_match = import_regex.exec(input)) !== null) {
const imports = import_map.get(import_match[2]) || new Set();
imports.add(import_match[4]);
import_map.set(import_match[2], imports);
}
// Replace inline imports with their type names
const transformed = input.replace(import_regex, "$4");
// Remove/adjust @template, @param and @returns lines
const lines = transformed.split("\n");
const filtered_lines = lines.filter(line => {
line = line.trim();
return !line.startsWith("* @template") &&
!((line.startsWith("* @returns") || line.startsWith("* @param")) && line.endsWith("}"));
}).map(line => {
line = line.trim();
// Strip {...} from @param and @returns lines
if (line.startsWith("* @param {") || line.startsWith("* @returns {")) {
let openCount = 1;
let closedCount = 0;
let i = line.indexOf('{') + 1;
for (; i < line.length; i++) {
if (line[i] === "{") openCount++;
if (line[i] === "}") closedCount++;
if (openCount === closedCount) break;
}
line = line.slice(0, line.indexOf('{')) + line.slice(i + 1);
}
return line;
});
// Replace generic type names with their plain versions
const renamed_generics = filtered_lines.map(line => {
return line.replace(/(\W|\s)([A-Z][\w\d$]*)_\d+(\W|\s)/g, "$1$2$3");
});
// Generate the import statement for the types used
const import_statements = Array.from(import_map.entries())
.map(([path, types]) => `import { ${[...types].join(', ')} } from '${path}';`)
.join("\n");
return [import_statements, ...renamed_generics].join("\n");
}| * return an error response without invoking `handleError`. | ||
| * Make sure you're not catching the thrown error, which would prevent SvelteKit from handling it. | ||
| * @param {number} status The [HTTP status code](https://developer.mozilla.org/en-US/docs/Web/HTTP/Status#client_error_responses). Must be in the range 400-599. | ||
| * @param {{ message: string } extends App.Error ? App.Error | string | undefined : never} message An object that conforms to the App.Error type. If a string is passed, it will be used as the message property. |
There was a problem hiding this comment.
This is not the same as the original definition, you need to define the overload as in https://devblogs.microsoft.com/typescript/announcing-typescript-5-0-beta/#overload-support-in-jsdoc, for which we need to update the TS in the repo to 5.0
There was a problem hiding this comment.
gah, this causes the build to fail because suddenly it doesn't know what App is (even though it was fine in the original definition). any ideas?
to be honest I'm still unclear on why we need the overload
There was a problem hiding this comment.
ah nvm figured it out, it was unrelated — i had removed .ts files from tsconfig.build.json
There was a problem hiding this comment.
You need the overload because the type definition is:
- if you did not modify
App.Error, in other words only{ message: string }is required, then the second parameter is optional (because it is falling back toError: ${status}in that case) - if you did modify
App.Error, in other words more than just{ message: string }is required, then you can't omit the second parameter and it is required
You can only define this with overloads
There was a problem hiding this comment.
gotcha, i missed the fact that one was optional and one wasn't. updated
packages/kit/src/exports/index.js
Outdated
| * @param {Record<string, any> | undefined} [data] | ||
| * Create an `ActionFailure` object. | ||
| * @param {number} status The [HTTP status code](https://developer.mozilla.org/en-US/docs/Web/HTTP/Status#client_error_responses). Must be in the range 400-599. | ||
| * @param {Record<string, any> | undefined} [data] Data associated with the failure (e.g. validation errors) |
There was a problem hiding this comment.
This is not the same function definition, you need to use @template to define the generic so it returns properly. It's probably also good explicitly annotate the return type (also on the other functions).
There was a problem hiding this comment.
updated, but i think i screwed it up somehow: #9895 (comment)
packages/kit/src/exports/index.js
Outdated
| * Create an `ActionFailure` object. | ||
| * @template {Record<string, any> | undefined} T | ||
| * @param {number} status The [HTTP status code](https://developer.mozilla.org/en-US/docs/Web/HTTP/Status#client_error_responses). Must be in the range 400-599. | ||
| * @param {T} [data] Data associated with the failure (e.g. validation errors) | ||
| * @returns {ActionFailure<T>} |
There was a problem hiding this comment.
There's something about this change that's causing one test to fail — the ActionResult here...
...has this type:
ActionResult<{
status: number;
data: {
fail: string;
} | undefined;
} | {
success: boolean;
} | {
id: number;
username: string;
profession: string;
} | {
status: number;
data: {
reason: {
error: {
code: "VALIDATION_FAILED";
};
};
} | undefined;
} | {
...;
}, never>It should have this type:
ActionResult<{
success: boolean;
} | {
id: number;
username: string;
profession: string;
}, {
fail: string;
} | {
reason: {
error: {
code: "VALIDATION_FAILED";
};
};
}>In other words, the ExcludeActionFailure and ExtractActionFailure types in the generated $types.d.ts are misfiring:
type ExcludeActionFailure<T> = T extends Kit.ActionFailure<any> ? never : T extends void ? never : T;
type ActionsSuccess<T extends Record<string, (...args: any) => any>> = { [Key in keyof T]: ExcludeActionFailure<Awaited<ReturnType<T[Key]>>>; }[keyof T];
type ExtractActionFailure<T> = T extends Kit.ActionFailure<infer X> ? X extends void ? never : X : never;
type ActionsFailure<T extends Record<string, (...args: any) => any>> = { [Key in keyof T]: Exclude<ExtractActionFailure<Awaited<ReturnType<T[Key]>>>, void>; }[keyof T];
type ActionsExport = typeof import('../../../../../../../../+page.server.js').actions
export type SubmitFunction = Kit.SubmitFunction<Expand<ActionsSuccess<ActionsExport>>, Expand<ActionsFailure<ActionsExport>>>Instead of getting a SubmitFunction<Success, Failure> we're getting something like a SubmitFunction<Success & Failure, never>. I'm not sure why.
There was a problem hiding this comment.
One way to solve the auto completion problem would be to not use typesVersions and instead create d.ts files which TypeScript will naturally pick up:
kit
hooks.d.ts -> just reexports everything from types/../hooks/index.ts or wherever
vite.d.ts -> same
node/index.d.ts -> same
node/polyfills.d.ts -> same
This pollutes the project a bit but I'm willing to take that hit for better DX.
In general my gripe with this change is that things are now split up. Interfaces are at X, methods at Y, it's all a bit scattered - maybe I'm just not used to it. This is the one thing TS makes better IMO, you can colocate this stuff better.
| @@ -18,8 +16,9 @@ import { | |||
| RequestOptions, | |||
| RouteSegment, | |||
| UniqueInterface | |||
| } from './private.js'; | |||
| } from './private'; | |||
There was a problem hiding this comment.
AFAIK all type imports need the .js ending or else moduleResolution: node16/nodenext will error. It's stupid, but it is what it is.
| export class Server { | ||
| constructor(manifest: SSRManifest); | ||
| export interface Server { | ||
| new (manifest: SSRManifest): Server; |
There was a problem hiding this comment.
Why this change? Curious if these are equivalent.
There was a problem hiding this comment.
ah this is because at one point i was experimenting with having .ts files in src instead of .d.ts files, so that all the .d.ts files would be generated. (i think having .d.ts files in src is better, on reflection — we just need to copy them over to the types directory). you can't declare a class in a .ts file without actually implementing it
now that it's in a .d.ts again we could revert
Unfortunately VSCode is liable to create broken imports like this... import { thing } from '@scope/package/subpackage/index.js';...when you want this: import { thing } from '@scope/package/subpackage';It's dependent on the user's config etc, so it's unreliable.
TS definitely is better on this dimension, though I actually think this PR is better than what we currently have — all the types are in Though it is a shame that we have to use |
|
After playing around a bit more I've come to the conclusion that what we really want is to generate (from source, with sourcemaps) a single declare namespace App {
export interface Error {...}
}
declare module '@sveltejs/kit' {
export interface Adapter {...}
export function error(status: number, body: App.Error): ...
// etc
}
declare module '@sveltejs/kit/hooks' {
import { Handle } from '@sveltejs/kit';
export function sequence(...handlers: Handle[]): Handle;
}
declare module '$app/environment' {
export const browser: boolean;
// ...
}
// etc
//# sourceMappingURL=index.d.ts.mapThe This is the most reliable way to get auto-imports working across all TypeScript configurations, and is the only way to make it work with modules like Unfortunately, a tool for creating such a |
|
going to close this and maybe take another run at it later |
Another run at #9883. We can't use .js files as the source of truth inside a package in
node_modules, but we can use them as the source of truth for the.d.tsfiles. Combined with thedeclarationMapoption, this makes 'go to definition' work as expected, and means that we don't have interfaces and documentation split between.d.tsand.jsfiles.This is WIP but I'm feeling fairly good about it. Downsides:
tscif we change interfaces while developing (but we still get HMR etc for non-interface changes, which is most of them)we need to put types that can't be expressed in JSDoc in.tsfiles, but in order to retain the benefits of shipping raw source code, we need to ensure that we only export types, not values, from those files. It might be worth having a build time check to guarantee thatUpdate
Ok, so this mostly works (though the logic for generating docs from types needs to be updated). Summary:
.jsfilestscto turnsrc/**/*.jsintotypes/**/*.d.ts(and*.d.ts.map, so 'go to definition' goes to the source)src/**/*.d.tstotypesimport('types')stuff to use relative imports, otherwise types will be broken for people not usingmoduleResolution: 'bundler'pkg.exportspoints at the source files, as before, but the.export additionally has atypescondition which points attypes/exports/types.d.ts(copied fromsrc/exports/types.d.ts)pkg.typesandpkg.typesVersionsare configured so that all this continues to work withoutmoduleResolution: 'bundler'Trade-offs:
@sveltejs/kit/*, only@sveltejs/kititself. This is rather annoying. I'm not sure if there's a piece missing that would fix this. We could generate thedeclare moduledeclarations from the.d.tsfiles, I suppose, though that would kinda suckPlease 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:.