-
Notifications
You must be signed in to change notification settings - Fork 382
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
perf: optimize createUpgradableConstructor
#4092
perf: optimize createUpgradableConstructor
#4092
Conversation
// Perf optimization - the vast majority of components have formAssociated=false, | ||
// so we can skip the setter in those cases, since undefined works the same as false. | ||
// @ts-expect-error type-mismatch | ||
UpgradableConstructor.formAssociated = isFormAssociated; |
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.
`<${tagName}> was already registered with formAssociated=${UpgradableConstructor.formAssociated}. It cannot be re-registered with formAssociated=${isFormAssociated}. Please rename your component to have a different name than <${tagName}>` | ||
); | ||
} | ||
|
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.
This is not a perf optimization – I just felt it was odd to have this logic inside of the try
/finally
, since it doesn't strictly need to be there. 🙂
Note that the UpgradableConstructor.formAssociated
now has to be cast to a Boolean
because it may be undefined
.
callback(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.
This class is basically hoisted unmodified from the old UpgradableConstructor
. The only difference is that it's missing the formAssociated
, which has to be different per-class.
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!
Details
This optimizes
createUpgradableConstructor
and thus improves thedom-light-styled-component-create-1k-different
benchmark by 6-7%:The main optimization is to avoid creating the same
class
with the same methods/constructor over and over, and to instead justextend
a base class.Does this pull request introduce a breaking change?
Does this pull request introduce an observable change?