[POC] feat: add support for session refresh for safari #266
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #266 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 14 14
Lines 626 641 +15
Branches 162 170 +8
=========================================
+ Hits 626 641 +15 ☔ View full report in Codecov by Sentry. |
src/identity.js
Outdated
| const client_sdrn = `sdrn:${NAMESPACE[this.env]}:client:${this.clientId}`; | ||
| const redirectBackUrl = this.redirectUri.substring(0 , this.redirectUri.lastIndexOf('/')); | ||
| const params = { redirect_uri: redirectBackUrl, client_sdrn: client_sdrn}; | ||
| this.emit('redirectToSessionService'); |
There was a problem hiding this comment.
Honestly, this event has no sense in this place because we are redirected right after the event is triggered.
For PoC it can stay, but not for final solution
There was a problem hiding this comment.
Wouldn't this event allow the brands to save any progress on critical flows to i.e. localStorage before the redirect happens?
hunger-programmer
changed the title
hunger-programmer
force-pushed
the
feat/UUI-683/add-support-for-page-refresh
branch
5 times, most recently
from
3499cfb to
9f50c25
Compare
ref: UUI-683
hunger-programmer
force-pushed
the
feat/UUI-683/add-support-for-page-refresh
branch
from
9f50c25 to
8b513b0
Compare
JIRA: UUI-873
.github/workflows/npm-publish.yml
Outdated
| @@ -38,6 +38,6 @@ jobs: | |||
| - name: Build application | |||
| run: npm run build | |||
| - name: Publish to npm | |||
| run: npm publish --access=public | |||
| run: npm publish --access=public --tag beta #TODO: To be removed, for PoC purposes only | |||
There was a problem hiding this comment.
Beta tag should be removed for final release
src/identity.js
Outdated
| // for expiring session and safari browser do full page redirect to gain new session | ||
| if(_checkRedirectionNeed(sessionData)){ | ||
| if (this._enableSessionCaching) { | ||
| const expiresIn = 1000 * (sessionData.expiresIn || 300); |
There was a problem hiding this comment.
In this case, sessionData is an object with one key "redirectURL".
We should consider creating a separate field for this.
src/identity.js
Outdated
| if (sessionData){ | ||
| // for expiring session and safari browser do full page redirect to gain new session | ||
| if(_checkRedirectionNeed(sessionData)){ | ||
| if (this._enableSessionCaching) { |
There was a problem hiding this comment.
Can this "semaphore" logic be extracted into separate methods?
hasSession does a lot already.
src/identity.js
Outdated
| return _postProcess(sessionData); | ||
| }; | ||
| this._hasSessionInProgress = _getSession() | ||
| .then( | ||
| sessionData => { | ||
| this._hasSessionInProgress = false; | ||
|
|
||
| if (typeof sessionData === 'string' && isUrl(sessionData)) { |
There was a problem hiding this comment.
typeof sessionData === 'string' can be removed.
If sessionData is not a stringified URL isUrl will safely fail to parse it and return false
src/identity.js
Outdated
| @@ -480,9 +491,16 @@ export class Identity extends EventEmitter { | |||
| * @return {Promise<HasSessionSuccessResponse|HasSessionFailureResponse>} | |||
| */ | |||
| hasSession() { | |||
| const checkIfSessionOngoing = this.cache.get("sessionFlowOngoing"); | |||
| if (checkIfSessionOngoing) | |||
There was a problem hiding this comment.
Please remove newline before '{'
bogdan-niculescu-sch
deleted the
feat/UUI-683/add-support-for-page-refresh
branch
ref: UUI-683