Skip to content
This repository was archived by the owner on Oct 4, 2023. It is now read-only.

[C-832] Add local-storage service to saga context#1718

Merged
dylanjeffers merged 9 commits into
mainfrom
dj-c-832-make-local-storage-service-native-compatible
Aug 16, 2022
Merged

[C-832] Add local-storage service to saga context#1718
dylanjeffers merged 9 commits into
mainfrom
dj-c-832-make-local-storage-service-native-compatible

Conversation

@dylanjeffers

Copy link
Copy Markdown
Contributor

Description

Adds local-storage service to saga context.
Updates getAccount usages to wait until account is loaded (since it is now async) before requesting it

Dragons

There may be subtle places where we are meaning to request data with an authenticated user, but instead are doing the public fetch. There may be other places that are expecting an account right away even though it is still loading. I believe I have covered the majority of places

How Has This Been Tested?

Running staging and prod and going through the major flows.

How will this change be monitored?

Looking for any issues in sentry that result from this change, as well as responding to any internal feedback of loading issues.

@sliptype sliptype left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good to me! A couple type errors


// Init APICLient
yield* call(() => apiClient.init())
apiClient.init()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why remove the yield here when we made init async?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

oh yeah i can bring that back, i actually didnt make it async since it was messing up a lot of places and for very little upside, which is just to init the eager discovery provider, which is only a web thing anyway.

localStorage: LocalStorageType
}

export class LocalStorage {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Did you make this a class instead of a function for consistency with the other services?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yeah i guess so, happy to make a function if we think it makes more sense though.

waitForValue,
(state) => state.account.status,
null,
(status) => status !== Status.LOADING

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yeah this felt good, thank the lord, lol

@audius-infra

Copy link
Copy Markdown
Collaborator

@dylanjeffers dylanjeffers merged commit f5daf2b into main Aug 16, 2022
@dylanjeffers dylanjeffers deleted the dj-c-832-make-local-storage-service-native-compatible branch August 16, 2022 22:08
@sliptype sliptype mentioned this pull request Aug 18, 2022
@AudiusProject AudiusProject deleted a comment from linear Bot Sep 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants