Update session timeout poll response handling to omit timeout on inactive session#8521
Update session timeout poll response handling to omit timeout on inactive session#8521
Conversation
Warden will automatically set a response header 401 if the request includes a cookie for an expired session. Because our request utility throws on non-200s, we need to catch the error.
| live, | ||
| timeout, | ||
| }); | ||
| }: R): SessionLiveStatus | SessionTimedOutStatus { |
There was a problem hiding this comment.
The types here were giving me trouble where I was hoping TypeScript would be a bit smarter about resolving between true & false to narrow the type, but it wasn't working the way I'd hoped.
| await request('https://example.com', { read: false }) | ||
| .then(() => { | ||
| throw new Error('Unexpected promise resolution'); | ||
| }) | ||
| .catch((error) => { | ||
| expect(error).to.exist(); | ||
| }); |
There was a problem hiding this comment.
This test wasn't effective in testing that the request will throw, since the fallback handling for "unexpected promise resolution" is what was being caught, not the error thrown by request.
| timeout?: undefined; | ||
| } | ||
|
|
||
| export type SessionStatus = SessionLiveStatus | SessionTimedOutStatus; |
There was a problem hiding this comment.
how come have both SessionStatus and SessionStatusResponse? I feel like having the union type is useful for the actual TS code, since the code is already mapping responses from the server explicitly, having the union type for SessionStatusResponse seems like it doesn't give us much?
There was a problem hiding this comment.
It was probably useful when I was trying different implementations to get the type narrowing to cooperate with different generic forms. I suspect I left it mostly as a nice symmetry between the "input" and "output" types. Ultimately though, yeah, it's unused, and if anything the overloaded function generic using it would be clearer by just spelling out the union of overloaded forms explicitly.
There was a problem hiding this comment.
yeah if I was writing this I'd probably collapse the "raw" server types into a singular one, and use the union type for the one passed around in TS code
There was a problem hiding this comment.
Hm, I think I may have initially misunderstood the suggestion. But after digging in, both interpretations lead me to think that maybe it's worth keeping as-is:
- If the idea is to remove
SessionStatusResponseas a union ofSessionLiveStatusResponse | SessionTimedOutStatusResponse, while keeping the latter types, this makes sense for the overloaded method signature, but makes therequestgeneric parameter below a little less expressive. - If the idea is to flatten
SessionStatusResponseas the interface and update property types to reflect all forms (live: boolean; timeout: string | null), this makes TypeScript unable to do type narrowing in the logic of the mapping function, specifically on calling theDateconstructor with a possibly-nulltimeout(screenshot).
As a compromise, I'd suggest keeping the union type for the request calls, and updating the overloaded signature to re-reference the individual response types, to reflect the overloaded forms. See 0b0efb5.
There was a problem hiding this comment.
yeah I had intended the "flatten SessionStatusResponse as the interface and update property types to reflect all forms" approach, but the screenshot makes a good case for keeping. Thanks for trying!
This reverts commit fd2d313.
2 participants
🛠 Summary of changes
This supports #7966 and #8507, with the goal of improving the resiliency of how a timed-out session behaves:
mainwith including a small offset, it could run risk of drift between server and frontend. The frontend doesn't currently do anything withtimeoutexcept in the case that the session is still live, so this is not likely a problem for the moment.An alternative approach could be to adopt something like how it behaves in main with the small offset, which does simplify by always being able to assume that
timeoutis present and a string in the response. It's unclear if server/client drift would ever actually be a problem, or if it's safe to assume that any drift would always be in the direction of making the session more timed-out.