Skip to content

Conversation

@MieszkoTSH
Copy link
Collaborator

I have changed authStorage to be the singe source of truth for auth tokens data. I tried to keep it pretty close to what we had before, but i have also added some methods that simplify e.g. reseting token data

@changeset-bot
Copy link

changeset-bot bot commented Jun 24, 2024

🦋 Changeset detected

Latest commit: e74933d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
react-starter-boilerplate Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@MieszkoTSH MieszkoTSH force-pushed the feat/auth-storage-sync-store branch from 2d8603b to 21fbc5f Compare June 25, 2024 10:19
Comment on lines 21 to 23
accessToken: _storage.getItem(ACCESS_TOKEN_KEY),
refreshToken: _storage.getItem(REFRESH_TOKEN_KEY),
expires: Number(_storage.getItem(EXPIRES_KEY)),
Copy link
Collaborator

Choose a reason for hiding this comment

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

how does this constructor with reading data from storage work with the defaultTokenData in line 14?

private _refreshToken: string | null = null;
private _expires: number | null = null;
private _storage: Storage | null = null;
private _storage: Storage | null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Explicit typing can be removed, a type will be infered based on the assignment in the constructor

Suggested change
private _storage: Storage | null;
private _storage;

private _expires: number | null = null;
private _storage: Storage | null = null;
private _storage: Storage | null;
private _tokenData: TokenData = defaultTokenData;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this typing and default value can be omitted. This comment is related to the question here https://github.com/TheSoftwareHouse/react-starter-boilerplate/pull/207/files#r1664081100

@tokerson
Copy link
Collaborator

tokerson commented Jul 3, 2024

Removing authReducer now causes errors in this file src/context/auth/authContext/AuthContext.types.ts, looks like we are not running a npm run typecheck in the CI cause it didn't catch this error

@MieszkoTSH
Copy link
Collaborator Author

MieszkoTSH commented Jul 3, 2024

@tokerson

Removing authReducer now causes errors in this file src/context/auth/authContext/AuthContext.types.ts, looks like we are not running a npm run typecheck in the CI cause it didn't catch this error

I have missed that 😅.
We have also merged some depndabot update with type errors (#209 fixed in this PR), so we might want to consider adding type checking to CI

this._tokenData = {
accessToken: _storage.getItem(ACCESS_TOKEN_KEY),
refreshToken: _storage.getItem(REFRESH_TOKEN_KEY),
expires: Number(_storage.getItem(EXPIRES_KEY)),
Copy link
Collaborator

Choose a reason for hiding this comment

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

if EXPIRES_KEY does not exist then this line will be Number(null) which will be evaluated to 0, therefore the default value for expires will be 0, which is different than expires in the defaultTokenData object.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we'd need to do sth like expires: _storage.getItem(EXPIRES_KEY) ? Number(_storage.getItem(EXPIRES_KEY)) : defaultTokenData.expires,

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yup you are right, fixed that

@MieszkoTSH MieszkoTSH force-pushed the feat/auth-storage-sync-store branch from 5798006 to e74933d Compare July 4, 2024 08:43
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.

3 participants