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

Add type definitions for parseOptions in FastifyCookieOptions #161

Closed
2 tasks done
MoSheikh opened this issue Jan 1, 2022 · 4 comments · Fixed by #162
Closed
2 tasks done

Add type definitions for parseOptions in FastifyCookieOptions #161

MoSheikh opened this issue Jan 1, 2022 · 4 comments · Fixed by #162
Labels
bug Confirmed bug good first issue Good for newcomers

Comments

@MoSheikh
Copy link
Contributor

MoSheikh commented Jan 1, 2022

Prerequisites

  • I have written a descriptive issue title
  • I have searched existing issues to ensure the feature has not already been requested

🚀 Feature Proposal

Currently, parseOptions is not defined under FastifyCookieOptions. I was hoping to change the FastifyCookieOptions interface to the following:

// plugin.d.ts

export interface FastifyCookieOptions {
  secret?: string | string[];
  parseOptions?: CookieSerializeOptions;
}

And add the following test:

// test/plugin.d.ts

const appWithParserOptions = fastify();

const parseOptions: fastifyCookieStar.CookieSerializeOptions = {
  domain: "example.com",
  encode: (value: string) => value,
  expires: new Date(),
  httpOnly: true,
  maxAge: 3600,
  path: "/",
  sameSite: "lax",
  secure: true,
  signed: true,
};
expectType<fastifyCookieStar.CookieSerializeOptions>(parseOptions);

appWithParserOptions.register(cookie, {
  secret: "testsecret",
  parseOptions,
});
appWithParserOptions.after(() => {
  server.get("/", (request, reply) => {
    const { valid, renew, value } = reply.unsignCookie(request.cookies.test);

    expectType<boolean>(valid);
    expectType<boolean>(renew);
    expectType<string | null>(value);
  });
});

Motivation

No response

Example

No response

@mcollina
Copy link
Member

mcollina commented Jan 1, 2022

Thanks! Would you like to send a Pull Request to address this issue?

@mcollina mcollina added bug Confirmed bug good first issue Good for newcomers labels Jan 1, 2022
@MoSheikh
Copy link
Contributor Author

MoSheikh commented Jan 1, 2022

Sure! I did try pushing a branch through but it gave me a Permission Denied error - would I need to be added as a contributer?

@mcollina
Copy link
Member

mcollina commented Jan 1, 2022

@MoSheikh
Copy link
Contributor Author

MoSheikh commented Jan 1, 2022

@mcollina Oh right - thank you! Here's the PR.

@darkgl0w darkgl0w linked a pull request Jan 1, 2022 that will close this issue
4 tasks
@Eomm Eomm closed this as completed in #162 Jan 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Confirmed bug good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants