LoadInput and LoadOutput should be public shapes

elliott-with-the-longest-name-on-github commented 2 years ago

Describe the problem

Related to #4138

At some point in the past few months, the LoadInput and LoadOutput interfaces became private. IMO, if these interfaces make up the public contract for Load, they themselves should be public. The specific issue this caused when I upgraded to the newest version of SvelteKit? This function became unusable with TypeScript:

const protectedPage = (pageInput: LoadInput, checkRoles: Array<AuthRole>, allBut = false): LoadOutput => {
    const session = pageInput.session as PublicAppSession;
    const userRoles = session.auth.roles;
    if (!_isAuthorized(userRoles, checkRoles, allBut) && pageInput.page.path !== '/auth/unauthorized') {
        return {
            status: 302,
            redirect: '/auth/unauthorized',
        };
    }
    return {
        status: 200,
        props: {
            session,
        },
    };
};

This was a super useful function which I now have to completely rework or abandon typing on.

Describe the proposed solution

Make the LoadInput and LoadOutput interfaces public again.

Alternatives considered

Abandon typing, refactor anything referencing these types, or mock the interface locally and type cast the input to the local interface.

Option 1 is a no-go for obvious reasons.

Option 2 is feasible, but annoying.

Option 3 is dangerous, as the interface could change on SvelteKit's end.

Importance

would make my life easier

Additional Information

No response

Rich-Harris commented 2 years ago

I'm on board with making these public, though I think we should probably take the opportunity to tidy things up. Instead of having separate Load and ErrorLoad (with LoadInput and ErrorLoadInput) we should unify them, and always expose status and error (both null in the default case, unless we're rendering a page with a validation error).

elliott-with-the-longest-name-on-github commented 2 years ago

Thanks for the feedback, @Rich-Harris.

I took a quick look into the source and this doesn't look like a super complicated change. I was able to identify the following:

Move types from private.d.ts to index.d.ts and combine LoadInput and ErrorLoadInput
Update the load handler in the server package
Update site documentation

I'm sure there's more around the second bullet point, but I'm on mobile, so I can't do a full search yet.

I'd be happy to make a fork and take a crack at this change if you guys are okay with a community contribution on this. Just let me know -- and if you would like me to look at it, I welcome your feedback on where any "gotchas" I wouldn't easily find might be.

Thanks!

elliott-with-the-longest-name-on-github commented 2 years ago

I realized this is simple enough that I don't really care if my work gets thrown out. 😃

I pushed changes to this branch: https://github.com/tcc-sejohnson/kit/tree/sejohnson-make-public-typings

I have not opened a pull request yet, because I haven't generated changeset or updated the documentation. I had a few questions around the changes.

As requested by @Rich-Harris, this was an opportunity to clean up the typing surface by merging ErrorLoad and Load and their corresponding ErrorLoadInput and LoadInput types. This was a simple change -- ErrorLoadInput simply extended LoadInput with two properties, status and error. I moved these properties into LoadInput with the following behavior:

If there is no error, status and error are null
If there is an error, status is coalesced to 500 (this mirrors behavior elsewhere in Kit)
If there is an error, the error is coalesced with coalesce_to_error
The load_input object is then passed to module.load as usual

Interestingly, there's no actual behavior change here -- prior to the typing change, the status and error properties would've been passed to module.load -- but they were not included in the typing for the Load function. Their addition in the LoadInput type makes them part of the public api -- which may or may not be what the Svelte team wants. If error is going to be set to internal values we don't want exposed publicly, then we probably should keep the ErrorLoadInput type as an additional, internal type to signal that, while the value passed to Load may contain status and error values, they shouldn't be publicly relied upon. (I'm specifically concerned about the possibility that people could rely upon the specific content of an error, such as a message, which may be changed internally.)

Then again, the load_input.status and load_input.error properties are never used after the module.load.call line, so maybe they were always intended to be public, but were accidentally omitted from the public type?

Other than the above question, everything else looks great -- I created a sandbox and linked my local kit to it and the typings came through just as expected.

Once this discussion point is wrapped up, I can open a pull request complete with documentation updates and a changeset.

Rich-Harris commented 2 years ago

Thanks! There should be no need for the ?? 500 or coalesce_to_error stuff — everything should already be normalized by this point. In fact, the if (error) and if (is_error) blocks are superfluous, I think — you could just set them on the load_input objects directly instead of initialising to null.

The errors that are surfaced here are intended to be the errors that are used for rendering an error page, i.e. they're already meant to be public and there's no problem exposing them (or rather, keeping them exposed — as you say, there's no behaviour change here, status and error are only non-null when we're rendering an error page). There's a separate discussion we've been having around making it possible to mask unexpected errors (e.g. handleError returns new Error('internal server error') to avoid leaking anything sensitive), but we don't need to worry about that here.

elliott-with-the-longest-name-on-github commented 2 years ago

Thanks Rich.

The reason for the ?? 500 and coalesce_to_error calls is because the function declaration marks the arguments as possibly undefined (so it's a type error to assign them directly to status and error directly, which aren't optional but are nullable). They have to be coalesced to either null (which feels... weird, as it opens up the possibility of an error with a null status code, or the other way around) or a concrete value. I don't think it's an option to mark the function arguments as required, either, because they're being called with spread arguments (and undefined errors, etc) elsewhere.

Let me play with it for a few and I'll see if there's a better way to do it. Given that the if (error) block seems to be superfluous, it may just be that we can directly assign to load_input and coalesce the arguments to null (so if they're undefined they become null. I'll report back.

elliott-with-the-longest-name-on-github commented 2 years ago

@Rich-Harris, not sure what kind of PR monitoring you guys do -- so just tagging you to let you know I opened #4515 for this. 🙂

johndunderhill commented 2 years ago

I second the request for a solution based on whatever approach you all decide is best. I'm also using the types, but currently have to hack the import from a private file.

import type { LoadInput, LoadOutput } from '@sveltejs/kit/types/private';

Appreciate the ongoing dedication to providing types.

elliott-with-the-longest-name-on-github commented 2 years ago

@johndunderhill

There's currently a pull request open to fix the issue altogether! You can read the few comments here and check out the change there for a complete picture -- but the tl;dr is that the ErrorLoad function signature has been combined with the Load function signature and LoadInput and LoadOutput have been made public. 🙂

Rich-Harris commented 2 years ago

closed via #4515

sveltejs / kit