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

Type definition of callback arguments for SessionStore.get #158

Open
2 tasks done
rktyt opened this issue Sep 2, 2022 · 6 comments
Open
2 tasks done

Type definition of callback arguments for SessionStore.get #158

rktyt opened this issue Sep 2, 2022 · 6 comments

Comments

@rktyt
Copy link

rktyt commented Sep 2, 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

  1. I would like to have the result value to be optional and allow null or undefined.
  2. I would like to have the method removed from the type definition of the result value.
    Probably only sessionId, encryptedSessionId and user-defined values are needed for the result value.
get(sessionId, callback) {
  getSessionDataFromCustomDatabase(sessionId)
    .then((data) => {
      if (!data) {
        // If the data simply does not exist, I want to do the following
        callback(null, null) // ❌ TS2345: Argument of type 'null' is not assignable to parameter of type 'Session'.
        // or 
        callback(null) // ❌ TS2554: Expected 2 arguments, but got 1.
        // or
        callback() // ❌ TS2554: Expected 2 arguments, but got 0.
        return
      }
      callback(null, data) // ⚠ If you want to follow the type definition, you need to implement touch, save, regenerate, and other methods on the data.
    })
    .catch((error: Error) => {
      // If the process fails, I want to do the following
      callback(error) // ❌ TS2554: Expected 2 arguments, but got 1.
      // or 
      callback(error, null) // ❌ TS2345: Argument of type 'null' is not assignable to parameter of type 'Session'.
    })
}

Currently, I have to do the following, but I feel it is because the original type definition is bad.

get(sessionId, callback) {
  getSessinDataFromCustomDatabase(sessionId)
    .then((data) => {
      if (!data) {
        callback(null, null as unknown as Fastify.Session) // Not at all type-safe.
        return
      }
      callback(null, data as Fastify.Session) // Not at all type-safe.
    })
    .catch((error: Error) => {
      callback(error, null as unknown as Fastify.Session) // Not at all type-safe.
    })
}

Motivation

No response

Example

No response

@Uzlopak
Copy link
Contributor

Uzlopak commented Sep 6, 2022

A PR is welcome. Dont forget to also adding typings tests.

It is planned to publish on friday a new version.

@Eomm
Copy link
Member

Eomm commented Sep 11, 2022

Released in v10

@Eomm Eomm closed this as completed Sep 11, 2022
@Uzlopak
Copy link
Contributor

Uzlopak commented Sep 11, 2022

I think what he wants ie that CallbackSession needs to have FastifySession and null as second param

@rktyt
Copy link
Author

rktyt commented Sep 12, 2022

Thanks.

I think what he wants ie that CallbackSession needs to have FastifySession and null as second param

Yes. And omit unnessary methods or properties.

Like this

// Note: I am not sure if the cookie property is necessary.
type UnnessaryRestoreSessionKeys = 'touch' | 'regenerate' | 'destroy' | 'reload' | 'save' | 'set' | 'get' | 'isModified' | 'cookie';
type CallbackSession = (err: Error | null, result: Omit<Fastify.Session, UnnessaryRestoreSessionKeys> | null) => void;

@Eomm Eomm reopened this Sep 12, 2022
@kgoedecke
Copy link

+1 for this.

@Uzlopak
Copy link
Contributor

Uzlopak commented Nov 1, 2022

@kgoedecke

Can you provide a PR?

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

No branches or pull requests

4 participants