Should ReadOnlyFormData type be exported?

[hgl]

Is your feature request related to a problem? Please describe.
Currently in the demo app, typescript complains about this method access:

kit/packages/create-svelte/templates/default/src/routes/todos/index.json.ts

Line 25 in 399220d

text: request.body.get('text')

Property 'get' does not exist on type 'BaseBody'.
  Property 'get' does not exist on type 'string'.

Describe the solution you'd like
The export statement should probably start with

export const post: RequestHandler<any, ReadOnlyFormData>

But that requires ReadOnlyFormData being able to be imported

Describe alternatives you've considered

How important is this feature to you?

Additional context

[hgl]

I'd be happy to send a PR, but I'm not sure if directly exporting a type from helper.d.ts in index.d.ts is something frowned upon.

[ignatiusmb]

That's probably just strict TS being TS. Although it's true that if we know ahead what the body type is, we can pass it in through its generics to avoid narrowing and writing code like this

if (typeof body !== 'string' && !(body instanceof Buffer)) {
  // body: ReadOnlyFormData
} else {
  // body: string | Buffer
}

Still, it's probably better to explicitly handle those cases where body would be string | Buffer and avoid Uncaught TypeError. If that's not preferable, a quick workaround would be to just use FormData in RequestHandler<any, FormData>. I try to avoid this as this would mean using any (which defeats the purpose of TS) or needing to create an explicit type for the first generic. Usually when expecting body of FormData, I would just return early to reduce indentation and still handle incorrect body types

export const post: RequestHandler = async ({ body }) => {
  if (typeof body === 'string' || body instanceof Buffer) {
    return { status: 400 }; // add your own custom error message
  }
  // code for body as ReadOnlyFormData
};

[hgl]

Explicitly exhausting body types is good advice, thanks.

I do hope svelte kit could somehow provide a way to more easily narrow the body to a specific type:

Instead of

if (typeof body === 'string' || body instanceof Buffer) {
    return { status: 400 };
}

We can just do

if (!(body instanceof ReadOnlyFormData)) {
    return { status: 400 };
}

In the previous case, if svelte kit ever decides to support additional body types (e.g., ReadableStream), it can break.

[jasonlyu123]

Alternatively, we can provide a type guard function to check instanceof so that the ReadOnlyFormData implementation can be kept internal.

Maybe it's not needed either. Since the user can also define a function by ton their own and check if methods exist. The concrete type doesn't matter to the user. The users only need to know if the object looks like it.

[jasonlyu123]

We can just do

if (!(body instanceof ReadOnlyFormData)) {
return { status: 400 };
}

To clarify and supplement my previous comment. I don't think the instanceof check in the users' code makes sense. That means the implementation class also needs to be exported. It needs to exist in the runtime.

I think it makes sense to export the interface ReadOnlyFormData. This allows annotating the shared code for handling form data. Though it's possible to use some utility types to extract the type, for example:

import type { BaseBody } from '@sveltejs/kit/types/helper';
type ReadOnlyFormData = Exclude<BaseBody, string | Buffer>

or just copy the interface into your own codebase. But I guess it may still have a lot of PR or issues in the future to ask for it to be exported. Like those store and transition types in the main repo.

[hgl]

I don't think the instanceof check in the users' code makes sense

Agreed, and I vote for exporting a type guard function.

Maybe it's not needed either. Since the user can also define a function

FormData has many methods with common names, userland's duck typing is probably going to be handwavy.

If this type guard is exported by svelte kit, it can export a function that directly does instanceof ReadOnlyFormData, much more accurate.

[hgl]

I also wonder, why not directly use FormData in svelte kit? Users can directly use instanceof FormData, and it also becomes possible to change the value and send to another api.

But I guess there is probably a reason I'm not aware of that calls for custom implementation.

[ignatiusmb]

I don't think the instanceof check in the users' code makes sense. That means the implementation class also needs to be exported. It needs to exist in the runtime.

I agree, and on that note, exporting a type guard function means @sveltejs/kit would need to implement and export it too, which just doesn't feel right.

Exporting ReadOnlyFormData might be the best option, but it's already placed in helper and only used internally there in BaseBody. Not to mention, string and Buffer are already available natively, so I think it makes sense to allow users to use FormData as is without importing an external type, while still getting the right methods (in the autocompletion).

[hgl]

First off, thanks @ignatiusmb for the PR.

However, on second thought, I'm in the opinion that this addresses the wrong problem.

An user agent can post any kind of data it wants to the endpoint, so a defense mechanism to check for body type as suggested in @ignatiusmb's first comment is really good.

The currently solution encourages people to directly use Response<any, FormData>, and wishfully think the body will be FormData-like.

I still think we should provide a mechanism to allow users to easily and explicitly check body type instead.

@Rich-Harris is it a bad idea to just use vanilla FormData instead of ReadableFormData? If some environment that svelte kit supports doesn't have FormData, maybe offer a class that does something similar to what node-fetch does for fetch?

[george-lim]

@ignatiusmb fix works for RequestHandler since that type already has the Body generic. I would like to do the same for the hook functions so that I can process form data from POST requests globally, like parsing CSRF tokens.