Skip to content
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.

Sign up for GitHub

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

Jump to bottom

potentially speed up hyphenating style names #3251

Merged
merged 1 commit into from
Sep 2, 2020
Merged

potentially speed up hyphenating style names #3251

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions packages/styled-components/src/utils/hyphenateStyleName.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,10 @@
* https://github.com/facebook/fbjs/blob/master/packages/fbjs/src/core/hyphenateStyleName.js
*/

const uppercasePattern = /([A-Z])/g;
const uppercaseCheck = /([A-Z])/;
const uppercasePattern = new RegExp(uppercaseCheck, 'g');
Copy link
Contributor

@quantizor quantizor Sep 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why this change would be meaningfully faster

Copy link
Contributor Author

@willheslam willheslam Sep 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For performance reasons it's using Regexp.prototype.test (as opposed to String.prototype.match) and quoting MDN:

As with exec() (or in combination with it), test() called multiple times on the same global regular expression instance will advance past the previous match.

It's a little unclear how it works so I've always avoided it - is each regex storing which strings it's been matched against?
Or is it a per-regex counter for all strings it's matched against?

let str1 = 'fooBarBaz'
let str2 = 'howAboutNow'
let regex = /([A-Z])/g
> regex.test(str1)
true
> regex.test(str1)
true
> regex.test(str1)
false
> regex.test(str1)
true
> regex.test(str2)
true
> regex.test(str1)
false

This seems to imply it's a per-regex counter, so by using a non-global version, the test check isn't internally changing its state.

As further proof of this, changing uppercaseCheck to /([A-Z])/g causes a load of tests to start failing as some names stop becoming hyphenated!

(If there's a better way to solve this problem I'm happy to change it)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, fair enough.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a quick note here for a minor change; with gzipping it's likely that this will end up being smaller if we just repeat the pattern (with the added g flag) but I realise that this was added in an attempt of deduplication.
I can also confirm that this can have a rather unexpected speed up, due to the reduced internal state, matching state, and lastIndex travelling over from the native irregexp engine.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, my original attempt also duplicated the regex, so if that's preferable I can put together a PR.

I have no idea if JS engines treat /foo/g equivalently to new RegExp(/foo/, 'g') or not - I appreciate that regex performance can be sensitive to slight changes.

const msPattern = /^ms-/;
const prefixAndLowerCase = (char: string): string => `-${char.toLowerCase()}`;

/**
* Hyphenates a camelcased CSS property name, for example:
Expand All @@ -25,8 +27,9 @@ const msPattern = /^ms-/;
* @return {string}
*/
export default function hyphenateStyleName(string: string): string {
return string
.replace(uppercasePattern, '-$1')
.toLowerCase()
.replace(msPattern, '-ms-');
return uppercaseCheck.test(string)
? string
.replace(uppercasePattern, prefixAndLowerCase)
.replace(msPattern, '-ms-')
: string;
}