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

(feat) SvelteKit typings without types #1918

Merged
merged 18 commits into from
Mar 7, 2023

Conversation

dummdidumm
Copy link
Member

@dummdidumm dummdidumm commented Mar 7, 2023

This allows you to omit the ./$types imports and still get proper typings, by inserting them under the hood. Example:

// +page.ts
export function load(event) {}
--->
export function load(event: import('./$types').PageLoadEvent) {}

This works for TS files, in Svelte files, and for svelte-check.

Works in special SvelteKit files (+page/layout(.server).ts/js) for all exports (actions/csr/ssr/prerender/trailingSlash/load)

@dummdidumm dummdidumm merged commit 0374000 into sveltejs:master Mar 7, 2023
@dummdidumm dummdidumm deleted the typings-without-types branch March 7, 2023 17:44
@dummdidumm
Copy link
Member Author

FYI @jasonlyu123 I merged this so we can "beta"-test it, you're still welcome to give this a review 👍

@fehnomenal
Copy link

fehnomenal commented Mar 8, 2023

Nice! Could this also work with export const load arrow functions like the extension generates? It does indeed work.


const originalLanguageServiceHost = info.languageServiceHost;

class ProxiedLanguageServiceHost implements ts.LanguageServiceHost {
Copy link
Member

Choose a reason for hiding this comment

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

There are still multiple methods needed to implement. For example, import path completion doesn't work as it needs readDirectory. Maybe we can create a Proxy and return the original object if we didn't override the method. But We might need more checks if something goes wrong.

new Proxy(originalLanguageServiceHost, {
        get(target, prop) {
            const proxied = languageServiceHost[prop as keyof ProxiedLanguageServiceHost];

            return proxied ?? target[prop as keyof ts.LanguageServiceHost];
        },
        set(target, prop, value) {
            (target as any)[prop as keyof ts.LanguageServiceHost] = value;
            return true;
        }
    }

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah just doing a proxy might be better, I'll try it out.

Copy link
Member Author

Choose a reason for hiding this comment

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

Tried, am running into what seems caching issues (the language service host in full capacity seems to do stuff with the diagnostics differently), so will only patch readDirectory for now

}

const languageServiceHost = new ProxiedLanguageServiceHost();
const languageService = ts.createLanguageService(
Copy link
Member

Choose a reason for hiding this comment

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

Wondering if there is a way to make this lazier. For example, only create if the target file is a page file. Creating a languageService increase quite a bit of memory usage.

I also tried reusing the documentRegistry used by TS Server. And it makes the extra language service impact way smaller. Memory usage went from 220MB to 160MB in my testing. It could be an optimization we can explore. Maybe we can also try reusing documentRegistry in the language server.

    const registry = ts.createDocumentRegistry();
    const originalRegistry = (originalLanguageServiceHost as any).documentRegistry;
    const proxyRegistry: ts.DocumentRegistry = {
        ...originalRegistry,
        acquireDocumentWithKey(
            fileName,
            path,
            compilationSettingsOrHost,
            key,
            scriptSnapshot,
            version,
            scriptKind,
            sourceFileOptions
        ) {
            if (kitPageFiles.has(basename(fileName))) {
                return registry.acquireDocumentWithKey(fileName, path, compilationSettingsOrHost, key, scriptSnapshot, version, scriptKind, sourceFileOptions);
            }

            return originalRegistry.acquireDocumentWithKey(fileName, path, compilationSettingsOrHost, key, scriptSnapshot, version, scriptKind, sourceFileOptions);
        },
        updateDocumentWithKey(
            fileName,
            path,
            compilationSettingsOrHost,
            key,
            scriptSnapshot,
            version,
            scriptKind,
            sourceFileOptions
        ) {
            if (kitPageFiles.has(basename(fileName))) {
                return registry.updateDocumentWithKey(fileName, path, compilationSettingsOrHost, key, scriptSnapshot, version, scriptKind, sourceFileOptions);
            }

            return originalRegistry.updateDocumentWithKey(fileName, path, compilationSettingsOrHost, key, scriptSnapshot, version, scriptKind, sourceFileOptions);
        }
    };

Copy link
Member Author

Choose a reason for hiding this comment

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

Damn this is a good find, thank you! I'll add this in a follow-up PR.

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

Successfully merging this pull request may close these issues.

None yet

3 participants