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

semi public types #4104

Merged
merged 7 commits into from
Feb 24, 2022
Merged

semi public types #4104

merged 7 commits into from
Feb 24, 2022

Conversation

Rich-Harris
Copy link
Member

@Rich-Harris Rich-Harris commented Feb 24, 2022

Deploy preview: https://kit-svelte-dev-git-semi-public-types-svelte.vercel.app/docs/types#additional-types


I tried to do this in #4021 but the merge was too gnarly. Easier to start fresh.

The basic thinking here is that we have public types in index.d.ts and ambient.d.ts, and private-but-documented types in private.d.ts. Changes to these types should be made carefully to avoid breakage.

internal.d.ts is for things that aren't referenced by public types, and can be changed freely.

So far I haven't actually moved most of the types, I've just set up the logic for extracting/rendering/linking them.

@changeset-bot
Copy link

changeset-bot bot commented Feb 24, 2022

🦋 Changeset detected

Latest commit: a9cb86e

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

@Rich-Harris
Copy link
Member Author

Ok I think this is ready for review. There's some further tweaks I'd like to make to the types (particularly EndpointOutput) but they probably warrant a separate PR.

The big change here, other than that private types are now documented, is that a bunch of formerly public types are now in private.d.ts. I've been quite liberal in moving things over, on the basis that we can easily move things from private.d.ts to index.d.ts, but we can't go the other way without breaking changes. The test for whether something goes in index.d.ts was 'is a user likely to need to import this type?'

Copy link
Member

@benmccann benmccann left a comment

Choose a reason for hiding this comment

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

this looks great!

type ToJSON = { toJSON(...args: any[]): Exclude<JSONValue, ToJSON> };

export type TrailingSlash = 'never' | 'always' | 'ignore';
// TODO should this be public?
Copy link
Member

Choose a reason for hiding this comment

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

maybe remove this TODO now that it's no longer public?

@Rich-Harris Rich-Harris merged commit ae6121e into master Feb 24, 2022
@Rich-Harris Rich-Harris deleted the semi-public-types branch February 24, 2022 16:46
This was referenced Feb 24, 2022
camargo added a commit to NASA-AMMOS/aerie-ui that referenced this pull request Feb 24, 2022
- Update Svelte Kit types to account for changes in
sveltejs/kit#4104
@frederikhors

This comment was marked as spam.

@tv42
Copy link

tv42 commented Mar 7, 2022

By unexporting LoadInput/LoadOutput, this change made using Typescript harder. Previously, I could say

  export async function load({ fetch }: LoadInput): Promise<LoadOutput> {

Now it seems I have to use this form

  export const load: Load = ({ fetch }) => {

I know this is a style thing, and a lot of people seem to write arrow expressions everywhere, but I'm more inclined to agree with https://deno.land/manual/contributing/style_guide#top-level-functions-should-not-use-arrow-syntax

@benmccann
Copy link
Member

I'm not disagreeing about whether we should export LoadInput and LoadOutput, but I would think if you want to avoid arrow expressions you could write it as:

  export const load: Load = function({ fetch }) {

@tv42
Copy link

tv42 commented Mar 7, 2022

Sure, that avoids the arrow expression, but still uses an unnecessary const where just export function should suffice.

JosephVolosin pushed a commit to NASA-AMMOS/aerie-ui that referenced this pull request Aug 20, 2024
- Update Svelte Kit types to account for changes in
sveltejs/kit#4104
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.

5 participants