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

Allow null Session in SessionCallback when there is an error. #180

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

kylerush
Copy link

@kylerush kylerush commented Dec 20, 2022

It looks like the existing code support an error being thrown on store.get, however, the type definition SessionCallback for the callback function on store.get requires a Session type variable for the session argument. This change would not cause the type check to fail when you call the store.get callback function like so:

callback(new Error("Some error"), null)

This change also exports the SessionCallback type on the FastifySession type so that we can specify that our store.get callback function is of type FastifySession.SessionCallback.

Checklist

types/types.d.ts Outdated Show resolved Hide resolved
@kylerush
Copy link
Author

Thanks @climba03003! I'm going to commit your suggestion and then would like to propose one more change to export the SessionCallback type. Will add that change shortly.

types/types.d.ts Outdated Show resolved Hide resolved
@kylerush kylerush requested review from Uzlopak and climba03003 and removed request for Uzlopak December 20, 2022 11:54
@kylerush kylerush requested review from Uzlopak and climba03003 and removed request for climba03003 and Uzlopak December 20, 2022 12:00
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@Uzlopak
Copy link
Contributor

Uzlopak commented Dec 24, 2022

@climba03003 PTAL

types/types.d.ts Outdated Show resolved Hide resolved
@kylerush
Copy link
Author

kylerush commented Dec 26, 2022

@climba03003 this new type definition seems to not be working, or at least I don't understand how it should work.

With the suggestion change, typescript tells me:

  1. Argument of type 'null' is not assignable to parameter of type 'Error'.
  2. Argument of type 'Session' is not assignable to parameter of type 'undefined'.

Screenshot 2022-12-26 at 11 51 43 AM

I am not quite sure how to solve this problem. Is seems this union type definition doesn't quite work for the use case. Any idea?

I tried to change it to an overloaded function, but that caused the arguments in the callback function accepts to lose their types:
Screenshot 2022-12-26 at 12 08 20 PM

@mcollina
Copy link
Member

CI is not happy

@kylerush
Copy link
Author

Yeah it needs a little more work. The new tuple type seems to be causing issues as currently implemented. Will try to knock this out tomorrow.

@jsardev
Copy link

jsardev commented Mar 27, 2023

Hey folks 👋 I see this one didn't get finished and I'd like to have it fixed as well.

I took some time to tinker around with CallbackSession type and found out that we don't really have a flawless solution for this case. Here are a few implementations that I tested out:

Intersection

type Callback = ( ((err: Error) => void) & ((err: null, result: number) => void) );

const getSession = (callback: Callback) => {
    callback(new Error()); // correct
    callback(new Error(), 5); // throws error, correct
    callback(null, 5); // correct
}

getSession((error) => {
    // parameters properly typed, TS compiles
    return;
})

getSession((error, result) => {
    // parameters properly typed, but TS does not compile
    return;
})

Function overload

type Callback = {
    (err: Error): void;
    (err: null, result: number): void;
}

const getSession = (callback: Callback) => {
    callback(new Error()); // correct
    callback(new Error(), 5); // throws error, correct
    callback(null, 5); // correct
}

getSession((error) => {
    // parameters properly typed, TS compiles
    return;
})

getSession((error, result) => {
    // parameters properly typed, but TS does not compile
    return;
})

You can see within the comments that those do not work as intended due to how TypeScript works. I think that the most optimal way we could go is this:

type Callback = {
    (err: Error, result?: undefined): void;
    (err: null, result: number): void;
}

const getSession = (callback: Callback) => {
    callback(new Error()); // correct
    callback(new Error(), 5); // throws error, correct
    callback(null, 5); // correct
}

getSession((error) => {
    // parameters properly typed but are not fully precise, TS compiles
    return;
})

getSession((error, result) => {
    // parameters properly typed but are not fully precise, TS compiles
    return;
})

This way we lose the preciseness of the arguments of the callback that we pass to the get function, but at least callback calls work as intended.

@kylerush Let me know if you have time to finish this off. If not, please let me know - I'll create a separate PR with the fix.

Also, let me know what you all folks think!

@mcollina
Copy link
Member

@sarneeh go ahead and open a fresh 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

Successfully merging this pull request may close these issues.

5 participants