-
Notifications
You must be signed in to change notification settings - Fork 73
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
Fix Notifications Infinite "NaN" Loop #686
Conversation
… loop and improve home page flickering
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
}; | ||
} | ||
|
||
export function useIosDevice() { |
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.
@calebjacob are these hooks, or would this be a better fit under utils
?
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.
A hook is the right call I think since these values need to be relied on in other useEffect()
and useCallback()
calls.
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.
We could still have useIosDevice()
but move detectIos()
to the utils
folder. Would you prefer that @charleslavon?
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.
No I don't feel strongly on that. Just trying to understand the differences in use/organization and make sure it's intentional
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.
Yes, let's move detectIos
to the utils
folder and make it exportable.
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.
But I'm also fine to go with this implementation
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.
Woops - sorry @shelegdmitriy, I didn't see your comment before I merged. Feel free to open a PR if you'd prefer to break out detectIos()
.
@@ -52,8 +52,9 @@ export function useBosLoaderInitializer() { | |||
useEffect(() => { | |||
if (loaderUrl) { | |||
fetchRedirectMap(loaderUrl); | |||
} else { | |||
} else if (flags) { |
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.
nice find 🔥
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.
preview looks good on desktop and mobile. Let's test on beta 👍🏾
Closes: #684
Our previous logic for detecting the iOS version would return
NaN
due to trying parse a number with multiple periods (EG:Number("16.2.3")
. In JS,NaN
is not referentially equal to itself, which was causing an infinite loop when used as auseEffect()
oruseCallback()
dependency.I broke out all of the notification alert logic from the home page into
<NotificationsAlert />
to help reduce re-renders of the entire activity page.I also realized that
useBosLoaderInitializer()
was prematurely settinghasResolved: true
- which would cause<VmComponent>
to fetch and render code from the blockchain before using theredirectMap
provided by BOS Loader for local development.I could use some help thoroughly testing the preview branch @charleslavon to make sure the issue is fully resolved and that I didn't break anything in the process. :)