-
Notifications
You must be signed in to change notification settings - Fork 188
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
Optimize injectAndGetClassName #213
Conversation
src/inject.js
Outdated
injectStyleOnce(className, `.${className}`, | ||
validDefinitions.map(d => d._definition), | ||
useImportant, selectorHandlers); | ||
const className = `${classNameBits.join("-o_O-")}` |
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 doesn't need to be in a template string, right?
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.
oops, nope.
src/inject.js
Outdated
const validDefinitions = styleDefinitions.filter((def) => def); | ||
|
||
let classNameBits = []; | ||
let definitionBits = []; |
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.
These can be const
, right?
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.
Oh yes, of course.
src/inject.js
Outdated
classNameBits.push(styleDefinitions[i]._name); | ||
definitionBits.push(styleDefinitions[i]._definition); | ||
} | ||
} |
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.
nit: 4 space indents, please
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.
gah! Would you be opposed to me setting up eslint to tell me about these things for this project?
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.
Hahaha not at all, I was thinking the same thing. We have eslint but it doesn't have much turned on. :P I think ideally it would follow the KA style eslint at https://github.com/Khan/khan-linter/blob/master/eslintrc but that's ginormous so maybe just adding in a few of the common annoying ones would be good...
This function is called every time css() is used, so we want it to be as fast as possible. I spotted three loops here that could be condensed into one. In my profiling this seems to make the function about 30% faster.
e2c558e
to
4199188
Compare
Updated! |
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.
LGTM!
Mmkay, gonna go make the release now. Wish me luck! |
This function is called every time css() is used, so we want it to be as
fast as possible. I spotted three loops here that could be condensed
into one. In my profiling this seems to make the function about 30%
faster.