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

Typing of ServerResponse doesn't jive with possibly undefined cookies (TypeScript) #1369

Closed
elliott-with-the-longest-name-on-github opened this issue May 6, 2021 · 5 comments · Fixed by #1375 or #3201

Comments

@elliott-with-the-longest-name-on-github
Copy link
Contributor

Describe the bug
Check out my implementation of a get endpoint:

export const get: RequestHandler<AppContext> = async ({ locals }) => {
	if (locals.auth.isAuthenticated) {
		const logoutUrl = provider.getSignoutUrl();
		locals.auth.isAuthenticated = false;
		persist(locals);
		return {
			status: 302,
			headers: {
				location: logoutUrl,
			},
		};
	} else {
		await destroy(locals.id);
		const setCookie = cookie.serialize('sessionId', '', {
			expires: new Date(0),
			httpOnly: true,
			path: '/',
    });
		return {
			status: 302,
			headers: {
				location: '/auth/farewell',
				'Set-Cookie': setCookie,
			},
		};
	}
};

This causes the following linting error:

Type '({ locals }: ServerRequest<AppContext, unknown>) => Promise<{ status: number; headers: { location: string; 'Set-Cookie'?: undefined; }; } | { status: number; headers: { location: string; 'Set-Cookie': string; }; }>' is not assignable to type 'RequestHandler<AppContext, unknown>'.
  Type 'Promise<{ status: number; headers: { location: string; 'Set-Cookie'?: undefined; }; } | { status: number; headers: { location: string; 'Set-Cookie': string; }; }>' is not assignable to type 'void | ServerResponse | Promise<ServerResponse>'.
    Type 'Promise<{ status: number; headers: { location: string; 'Set-Cookie'?: undefined; }; } | { status: number; headers: { location: string; 'Set-Cookie': string; }; }>' is not assignable to type 'Promise<ServerResponse>'.
      Type '{ status: number; headers: { location: string; 'Set-Cookie'?: undefined; }; } | { status: number; headers: { location: string; 'Set-Cookie': string; }; }' is not assignable to type 'ServerResponse'.
        Type '{ status: number; headers: { location: string; 'Set-Cookie'?: undefined; }; }' is not assignable to type 'ServerResponse'.
          Types of property 'headers' are incompatible.
            Type '{ location: string; 'Set-Cookie'?: undefined; }' is not assignable to type 'Headers'.
              Property ''Set-Cookie'' is incompatible with index signature.
                Type 'undefined' is not assignable to type 'string'.ts(2322)

The problem arises because 'Set-Cookie' is not set to anything on the first segment of the if statement. This seems like a pretty reasonable use-case -- I may want to set a cookie or not set a cookie based on application state, but the typing of ServerRequest doesn't seem to allow for different sets of conditional headers.

To Reproduce
Make an endpoint similar to mine. In one branch of an if statement, add a Set-Cookie header. In another, don't add a Set-Cookie header.

Expected behavior
Typing of ServerResponse allows for a non-static set of headers.

Severity
Low.

@Rich-Harris
Copy link
Member

Do you have a repo showing this issue? I can't reproduce it at all, and I don't understand why TS is giving you that error

@ignatiusmb
Copy link
Member

ignatiusmb commented May 7, 2021

This snippet should do it, put in any endpoint file

import type { RequestHandler } from '@sveltejs/kit';

export const get: RequestHandler = async ({ body }) => {
  if (body) {
    return {
      headers: {
        location: '/logout',
      },
    };
  } else {
    return {
      headers: {
        location: '/auth/farewell',
        'Set-Cookie': 'setCookie', // toggle error by commenting this line
      },
    };
  }
};

Edit: it doesn't work if strict is set to false (tsconfig.json), more specifically strictNullChecks

@elliott-with-the-longest-name-on-github
Copy link
Contributor Author

@Rich-Harris

Forgive me for reusing a repo, but here: https://github.com/tcc-sejohnson/svelte-node-build-error

That's the minimum I can use to get the error to show up. Maybe it has something to do with tsconfig? But I don't think I changed anything...

@ignatiusmb
Copy link
Member

Looks like this issue has resurfaced according to reports on Discord

@seanlail

This comment has been minimized.

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