CB-4436 update session only in touchSession #2377
Conversation
...gin-session-expiration/src/SessionExpireWarningDialog/SessionExpireWarningDialogBootstrap.ts
Outdated
Show resolved
Hide resolved
| @@ -95,6 +92,23 @@ export class SessionResource extends CachedDataResource<SessionState | null> { | |||
| return session; | |||
| } | |||
|
|
|||
| touchSession = () => { | |||
There was a problem hiding this comment.
please avoid the use of arrow functions. It's hard to debug
...gin-session-expiration/src/SessionExpireWarningDialog/SessionExpireWarningDialogBootstrap.ts
Outdated
Show resolved
Hide resolved
...ages/plugin-session-expiration/src/SessionExpireWarningDialog/SessionExpireWarningDialog.tsx
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| if (this.data?.valid) { | ||
| this.graphQLService.sdk.touchSession(); |
There was a problem hiding this comment.
This call will update the session lifetime. We need to read the session from this method and update setData of the resource like it was in refreshSilent
There was a problem hiding this comment.
I added this abstraction to avoid cyclic dependency
...gin-session-expiration/src/SessionExpireWarningDialog/SessionExpireWarningDialogBootstrap.ts
Outdated
Show resolved
Hide resolved
| try { | ||
| await this.sessionResource.touchSession(); | ||
| } catch (e) { | ||
| console.error('Session touch error', e); |
There was a problem hiding this comment.
It is forbidden to import notificationService into core-root, so I logged it with this
| try { | ||
| await this.sessionResource.updateSession(); | ||
| } catch (e) { | ||
| console.error('Session update error', e); |
There was a problem hiding this comment.
cannot import NotificationService here so just logged it
There was a problem hiding this comment.
you can move it to plugin-session-expiration (not the best place but it's ok)
| this.touchSessionTimer = setTimeout(() => { | ||
| if (this.touchSessionTimer) { | ||
| clearTimeout(this.touchSessionTimer); | ||
| this.touchSessionTimer = null; | ||
| } | ||
| }, SESSION_TOUCH_TIME_PERIOD); |
There was a problem hiding this comment.
please keep naming for constants also (notifyClientActivity)
| return; | ||
| } | ||
|
|
||
| const session = (await this.graphQLService.sdk.updateSession()).updateSession; |
There was a problem hiding this comment.
better to use object destructuring here
| try { | ||
| await this.sessionResource.updateSession(); | ||
| } catch (e) { | ||
| console.error('Session update error', e); |
There was a problem hiding this comment.
you can move it to plugin-session-expiration (not the best place but it's ok)
No description provided.