-
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 kebabify #207
Optimize kebabify #207
Conversation
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! just a small style nit to fix up :)
it('forces -ms-', () => { | ||
assert.equal(kebabifyStyleName('msFooBarBaz'), '-ms-foo-bar-baz'); | ||
}); | ||
}); |
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.
<3 <3
src/util.js
Outdated
if (result.slice(0, 3) === 'ms-') { | ||
return `-${result}`; | ||
} | ||
return 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.
nit: we use 4-space indents
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.
Done!
src/util.js
Outdated
|
||
export const kebabifyStyleName = (string /* : string */) /* : string */ => { | ||
const result = string.replace(UPPERCASE_RE, UPPERCASE_RE_TO_KEBAB); | ||
if (result.slice(0, 3) === 'ms-') { |
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.
Is this the fastest way to check this? It seems like checking result[0] === "ms" && result[1] === "s" && result[2] === "-"
might be faster than slicing the string, but is also really ugly. Maybe that's what .startsWith()
does, but not everything supports it yet...
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 microbenchmark shows bracket is probably the way to go. Not sure if it will really matter much in practice, but it is simple enough so I'll make the change.
https://jsperf.com/slice-vs-bracket-string-check/1
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 made a maybe-more-useful microbenchmark that actually tests strings that start with ms-
and it also agrees (much more strongly actually) so this looks great! :) https://jsperf.com/slice-vs-bracket-string-check-ms
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, in that case, maybe we should use a similar approach in importantify?
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 added a similar optimization to my importantify PR: bd0a575
My refactoring caused test coverage to drop, so I decided to add some unit tests here. They pass before and after my refactoring.
By passing a function to replace, we can do the uppercase to lowercase conversion at the same time. This is pretty much the exact example that MDN gives, with the only difference being that we aren't checking against the offset here to avoid adding "-" at the beginning of the string: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/replace#Using_an_inline_function_that_modifies_the_matched_characters In my profiling, this seems to make kebabify 50% faster (50ms -> 25ms).
Now that we've simplified kebabify, there really isn't much value in having it be in its own function like this. Moving it into kebabifyStyleName to simplify and optimize.
This avoids an often-unnecessary regex run, which makes this function run a little faster. I first went with str.slice, but benchmarking showed that to be 18% slower than bracket access.
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 now!
By passing a function to replace, we can do the uppercase to lowercase
conversion at the same time. This is pretty much the exact example that
MDN gives, with the only difference being that we aren't checking
against the offset here to avoid adding "-" at the beginning of the
string:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/replace#Using_an_inline_function_that_modifies_the_matched_characters
In my profiling, this seems to make kebabify 50% faster (50ms -> 25ms).