-
Notifications
You must be signed in to change notification settings - Fork 59
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(sdk): Improve useStorage and Optimistic revalidate effect #1186
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 246320b:
|
6dff439
to
c0e2c80
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! That article was a nice read, thanks.
Noticed this PR contains other unrelated commits from #1176, so the changes related to useStorage
are only in this last commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Woow, looks like the results were nice! 🚀
I notice some unrelated code as @filipewl mentioned. Could you please clean the PR?
Thank you for letting me know, i didn't notice. |
c0e2c80
to
ba81773
Compare
@filipewl @eduardoformiga fixed. |
ba81773
to
d607a05
Compare
@@ -45,11 +45,13 @@ export const useStorage = <T>(key: string, initialValue: T | (() => T)) => { | |||
const item = await getItem<T>(key) | |||
|
|||
if (!cancel && item !== null) { | |||
setData(item) | |||
setTimeout(() => { | |||
setData(item) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't we have racing conditions in here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ain't sure, but I will remove it.
packages/sdk/src/cart/Optimistic.tsx
Outdated
@@ -44,12 +44,16 @@ export const OptimisticProvider = <T extends Item = Item>({ | |||
|
|||
setIsValidating(false) | |||
if (newCart != null) { | |||
setCart(newCart) | |||
setTimeout(() => { | |||
setCart(newCart) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cant we have racing conditions in here ?
bb3cc65
to
246320b
Compare
What's the purpose of this pull request?
Improve the performance of useStorage hook.
How it works?
The useStorage hook has an effect that fetches an item from the indexedDB, then update an internal state with this item. So, exists two tasks, one is a micro-task because is a promise and the other one belongs to the same task. The problem is these two tasks sometimes can create tasks that took more than 50ms and are considered Long Tasks. Know this, this PR wraps the async promise and the set state inside a setTimeout. Why setTimeout, because it creates a new task. For more information about tasks and microtasks you can read this article
Take a look at the quantity of long tasks after the hydration
![image](https://user-images.githubusercontent.com/15680320/159784365-a7acb44b-b6e1-4041-ae54-fcad5effa2bb.png)
![image](https://user-images.githubusercontent.com/15680320/159784275-ce877e5c-88d6-445f-974f-db8bf6cb3266.png)
Before:
After:
Run PSI API for mobile 15 times and the result is this
Before: using production https://623b35ed844ee300083a6a4a--basestore.netlify.app/
Using https://sfj-86eb203--base.preview.vtex.app/
After: using https://deploy-preview-416--basestore.netlify.app/
Using https://sfj-b706982--base.preview.vtex.app/
How to test it?
Run PSI or run performance test using DevTools
base.store
Deploy Previewvtex-sites/base.store#416
References
https://jakearchibald.com/2015/tasks-microtasks-queues-and-schedules/