fix: improve resolver performance#623
fix: improve resolver performance#623prevwong merged 10 commits intoprevwong:mainfrom Traveller23:main
Conversation
🦋 Changeset detectedLatest commit: 722ab3b The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
prevwong
left a comment
prevwong
left a comment
There was a problem hiding this comment.
Thanks, looks good. Just left some nits
| let currResolver: Resolver | undefined = undefined; | ||
| const reversedResolver = new Map<React.ElementType | string, string>(); | ||
|
|
||
| export const resolveComponent = ( |
There was a problem hiding this comment.
nit:
type ReversedResolver = Map<React.ComponentType | string, string>;
type CachedResolverData = {
resolver: Resolver;
reversed: ReversedResolver;
}
let CACHED_RESOLVER_DATA: CachedResolverData | null = null;
const getReversedResolver = (resolver: Resolver): ReversedResolver => {
if ( CACHED_RESOLVER_DATA && CACHED_RESOLVER_DATA.resolver === resolver ) {
return CACHED_RESOLVER_DATA.reversed;
}
CACHED_RESOLVER_DATA = {
resolver,
reversed: new Map();
for ( const [name, comp] of Object.entries(resolver) {...}
}
const searchComponentInResolver = (
resolver: Resolver,
comp: React.ElementType | string
): string | null => {
const name = getReversedResolver(resolver).get(comp);
if (name !== null) {
return name;
}
if (typeof comp === 'string') {
return comp;
}
return null;
};| const resolvedName = resolver[componentName] | ||
| ? componentName | ||
| : searchComponentInResolver(resolver, comp); |
There was a problem hiding this comment.
nit: Think we can just simplify this to searchComponentInResolver in all cases since it won't really impact efficiency after the initial call:
const resolvedName = searchComponentInResolver(resolver, comp);
|
Okay, I'll change it in two days. |
|
@prevwong One question: if (typeof comp === 'string') {
return comp;
}It would save a lot of searching time if we just know that the type of the |
|
I'm done refactoring, please check it again. |
| const componentName = getComponentName(comp); | ||
| if (Object.hasOwn(resolver, componentName)) { | ||
| return componentName; | ||
| } | ||
|
|
||
| const name = getReversedResolver(resolver).get(comp); | ||
| return name !== undefined ? name : null; |
There was a problem hiding this comment.
Perhaps we can remove Line 41-44.
I think if the component doesn't exists in the reversed resolver, then we can assume that the component doesn't exist.
Even if the component's name/display name exist in the resolver, it might still belong to a different component, so this may lead to bugs
There was a problem hiding this comment.
That's true, I'll remove those lines now.
prevwong
changed the title
resolveComponent…component by its name
|
@prevwong All done. |
2 participants
After a certain number of components have been registered, rendering after deserialization becomes very slow, and this issue has been discussed many times on discord.
So I tried to solve this problem, and I finally realized that the root of the problem is the
resolveComponentmethod. In the original implemention, the time complexity of the search was O(N^2 * LogN), so as soon as the number of components increased, the time needed to search increased at a faster rate.I did two things: made the search time complexity O(LogN), and created a reverse index of the map and cached it.
This problem only occurs in the prodcution version, i.e. after the code has been compiled and converted to ES5 form. So I guess the problem is also related to the target javascript version.