-
-
Notifications
You must be signed in to change notification settings - Fork 99
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: Issue 32: Verbose logging about useLayoutEffect on server render with web-export with toast component #33
Conversation
the-simian
commented
Feb 5, 2024
- adds a line of code to prevent accessing function on server that causes verbose logging
- adds a prettierrc.json that matches the existing code style
…dd prettierrc.json to match single-quote-style in repo
This fixes #32 |
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.
Thank you for contributing!
Can you add this as a custom hook?
In ~/lib/useIsomorphicLayoutEffect.tsx
, can you add
import * as React from 'react';
export const useIsomorphicLayoutEffect = typeof window !== 'undefined' ? useLayoutEffect : useEffect
And then use it here?
FYI: If you were to use import * as React from 'react'
here, you would get the typescript error: "Cannot assign to 'useLayoutEffect' because it is a read-only property"
You bet, I think a custom hook is a better approach! I will say we unfortunately probably still need to use the 'shim' least for now. In this codebase for our own code we can use the isomorphic Specifically: React.useLayoutEffect(() => {
const newAnimationValue = isVisible ? 1 : 0;
animate(newAnimationValue);
}, [animate, isVisible]); I was considering opening a PR on that repo at some point in the future to fix it there. Basically submitting some sort of isomorphic hook solution like you've proposed. If the problem wasn't inside somone else's module I think I'd have done this first |
Update: I've added the hook for our own uses in situations where we'd call |
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.
Thank you for the clarifications
Here is the referenced issue in the other repo and I've made all the other requested changes: |
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.
Great! Thank you