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

[fix] improve type of init #2544

Merged
merged 2 commits into from
Oct 7, 2021
Merged

[fix] improve type of init #2544

merged 2 commits into from
Oct 7, 2021

Conversation

benmccann
Copy link
Member

@benmccann benmccann commented Oct 2, 2021

Closes #2249. There's still some desire to fix the TODO about whether there might be a better way to get the file path to the application, but that's outside the scope of #2249, so I think we can close the issue and consider the TODO separately

It turns out that init also doesn't need to expose so many options. They're just used internally for prerender and preview

@jthegedus I'm not sure if it's possible for you to test this to see if it allows you to finally use the types? I haven't really tried doing an npm/pnpm link to an external project

@changeset-bot
Copy link

changeset-bot bot commented Oct 2, 2021

🦋 Changeset detected

Latest commit: de8ba8c

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

@jthegedus
Copy link
Contributor

I can test this out today with my adapter, cheers for the work and keeping me updated with progress 😄

@jthegedus
Copy link
Contributor

I still can't apply these types on import, I have to type a re-assignment of App for VSCode to work properly.

import * as App from '../output/server/app.js';
import {toSvelteKitRequest} from './firebase-to-svelte-kit.js';

/** @type {import('@sveltejs/kit').App} */
const app = App;

app.init();

typing the import like this does not work for me:

/** @type {import('@sveltejs/kit').App} */
import * as App from '../output/server/app.js';

nor this:

import {init, render} from '../output/server/app.js';

/** @type {import('@sveltejs/kit').Init} */
init();

This reassignment is nothing though compared to the public API being correct, which I believe this change fixes, so thanks for that :)

@benmccann
Copy link
Member Author

I wouldn't have expected the last two to change, but I'm surprised the last one doesn't work. What do you see with the last one that isn't working?

@jthegedus
Copy link
Contributor

All the tooltips and go to definition just point to the import statement, not the type defined on the line above. I reloaded vscode a couple times to ensure it was restarting and ingesting the types correctly. Tooltip reads "import init". go to definition worked with clicking through the type itself. 🤷

@ignatiusmb
Copy link
Member

There's no other way to type an import other than having the definition from the import itself or using an ambient import, which we can't do both in this case since it's a relative path. The only workaround right now is to do a reassignment, which we can avoid by resolving the TODO

@jthegedus
Copy link
Contributor

This is a small api used by few people so having the reassignment as a "requirement" for proper typing of init and render. Can be resolved if/when the TODO is resolved

@benmccann benmccann changed the title [fix] expose init and render for adapters [fix] improve type of init Oct 7, 2021
@benmccann benmccann merged commit 2302a9e into master Oct 7, 2021
@benmccann benmccann deleted the init-render branch October 7, 2021 23:26
@github-actions github-actions bot mentioned this pull request Oct 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose adapter init & render types for Adapter Authors
3 participants