getSession should only accept one type of request/context

[dbrxnds]

Question 💬

Currently, you are able to pass in the request object to getSession by passing in the full context, or simply an object containing the context.req object. I don't understand why this is, especially as this makes the TS definitions less strict.
It is required to pass in the request object, but the current type defs do not reflect that.

Is there a reason why it is allowed to do it both ways rather than just forcing the consumer to pass in the req object as a required param?

How to reproduce ☕️

Use getSession in a TS environment and you'll see that it has no required params, even though it should at least require the req object.

Repro sandbox: https://codesandbox.io/s/sharp-sun-3lxoc?file=/pages/api/examples/session.ts

Contributing 🙌🏽

Yes, I am willing to help answer this question in a PR

[balazsorban44]

I haven't done the original implementation, but I am guessing it is for convenience only.

If I did the implementation, I probably would have opted to support either passing req or the whole context directly.

getSession(req)
getSession(ctx)

This

getSession({ctx})

looks confusing to me.

I don't think it is actionable right now though, and since it's not a serious problem either, I'm inclined to leave it as is. Do you have a suggestion for improvement?

[dbrxnds]

Looking at the source code I figured it was for convenience. My first thought was pretty much the same route as your first suggestion, however I noticed there are 2 other options available in this object. See here

As for suggestions on how to go about this, I think one of these two would be best:

getSession({ 
  req // Required
  ...optional options
})

Or

getSession(req, { ...optional options })

Personally I prefer the latter for the same reason as you mentioned, and I think most people will never use the other options, so it's just unnecessary overhead.

However I do think we should stick to only being allowed to pass the request object, as nothing else from the context is used. I am not sure if this has any actual real world advantages, but I personally find it to be a cleaner approach to only pass what is needed.

These would be breaking changes but i'd be happy to hear what others think.

[balazsorban44]

Ultimately, I think the server-side getSession might have to be separated from the front-end one (that does not need req) for this reason: #1535 (comment) In that case, we will actually need the res as well, since we set cookies.

[dbrxnds]

I wasn't aware of this thread. Thanks for linking. I completely overlooked the client side getSession in my initial proposal. I will look into this and see if I can create a PR out of it.

[balazsorban44]

I will close this in favour of #1535