-
Notifications
You must be signed in to change notification settings - Fork 49.6k
[playground] Fix useEffect on tabify #34594
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
return tabifyCache.get(key)!; | ||
} | ||
const result = tabify(store.source, compilerOutput, store.showInternals); | ||
tabifyCache.set(key, result); |
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.
Since you're using the store as a key this will continue growing forever (potentially on every keystroke) and include the whole store object. This should probably be some form of LRU cache that can clean up the old store keys and promises.
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.
Ah I missed that, thanks for the catch!
export const BASIC_OUTPUT_TAB_NAMES = ['Output', 'SourceMap']; | ||
|
||
const tabifyCache = new LRUCache<Store, Promise<Map<string, ReactNode>>>({ | ||
max: 100, |
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.
Holding 100 copies of the store still seems like a lot? How many do you really need 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.
Right–the cache should not be doing much aside from being used for the use
anyways, I'll lower it.
There was a bug in the Compiler Playground related to the "Show Internals" toggle due to a useEffect that was causing the tab names to flicker from a rerender. Rewritten instead with a `<Suspense>` boundary + `use`. DiffTrain build for [250f1b2](facebook@250f1b2)
There was a bug in the Compiler Playground related to the "Show Internals" toggle due to a useEffect that was causing the tab names to flicker from a rerender. Rewritten instead with a `<Suspense>` boundary + `use`. DiffTrain build for [250f1b2](facebook@250f1b2)
There was a bug in the Compiler Playground related to the "Show Internals" toggle due to a useEffect that was causing the tab names to flicker from a rerender. Rewritten instead with a `<Suspense>` boundary + `use`.
There was a bug in the Compiler Playground related to the "Show Internals" toggle due to a useEffect that was causing the tab names to flicker from a rerender. Rewritten instead with a
<Suspense>
boundary +use
.