-
Notifications
You must be signed in to change notification settings - Fork 24
#61 suspense samples #66
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
Conversation
| const AppInner: React.FC<WithStyles<typeof styles>> = ({classes}) => { | ||
| const [quote, setQuote] = React.useState<Quote>({ body: "", author: "" }); | ||
| const [picture, setPicture] = React.useState(); | ||
| const loadData = React.useCallback(() => { |
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.
Why did you use useCallback?
useCallback is to memoize the function by some params, but if you pass [] only memoize once.
Then, you are using it in useEffect but with [] as second parameter, that is, we are only calling loadData on componentDidMount.
Am I missing something?
Thank you.
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.
Cause I want to intentionally keep the same callback reference between re-renders. loadData does also serve as a handler for the button onClick.
|
|
||
|
|
||
| const Suspense: React.FC<{ tag: string, className?: string }> = ({ tag, className, children }) => { | ||
| const { promiseInProgress } = usePromiseTracker({ area: tag, delay: 0 }); |
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.
Maybe it's not necessary feed the delay equals 0.
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, you are right! Didn't realize it was optional.
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.
What a weird thing .... the .d.ts shipped with 2.0 version seems to have this:
interface Config {
area: string;
delay: number;
}
However, in src they are optional:
interface Config {
area?: string;
delay?: number;
}
Is this a bug?
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.
I have removed delay param anyway, but we should check the d.ts in v2.0 package as it seems to be not in sync with the one in src. @brauliodiez are you aware of this?
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.
I think in current build the are optional, I think it has been published (v 2.0.2)
interface Config {
area?: string;
delay?: number;
}
nasdan
left a comment
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.
Really cool, thank you. Only some comments about useCallback
No description provided.