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.

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 the performance of hyphenate method. #6274

Merged
merged 4 commits into from
Sep 5, 2017

Conversation

sliwey-zz
Copy link
Contributor

Modify the regular to make the replace method call only once to optimize
performance.

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

Other information:
This is a performance test: https://jsperf.com/vue-hyphenate

Hanks10100 and others added 2 commits July 24, 2017 07:32
* build(release weex): ignore the file path of entries

* [release] weex-vue-framework@2.4.2-weex.1
Modify the regular to make the `replace` method call only once to optimize
performance.
package.json Outdated
@@ -129,4 +129,4 @@
"path": "./node_modules/cz-conventional-changelog"
}
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

can you undo this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@posva Sorry about that, I have already undo this change :)

@posva
Copy link
Member

posva commented Aug 2, 2017

Didn't even know about \b!
I did some tests and it seems to work nicely 👍

sliwey added 2 commits August 2, 2017 18:03
Modify the regular to make the `replace` method call only once to optimize
performance.
@posva
Copy link
Member

posva commented Aug 2, 2017

Doing some perf checks and it looks like the old version is faster though 😆
edit ah nevermind, it looks like there's no difference

@sliwey-zz
Copy link
Contributor Author

@posva
I just do a simple test, and this is the link. It shows the new version is faster, but I only run the test in my environment (chrome 60.0.3112 + windows 10).
In addition, regular replacement once should be faster than twice, both the two regular are simple after all (I'm not sure if this is right).
So, how do you do the performance tests?
Sorry for maybe not having made my expression accurately, cause my English is not very good.

@HerringtonDarkholme
Copy link
Member

HerringtonDarkholme commented Aug 3, 2017

Technically this is a breaking change. What counts as a word is listed in http://www.ecma-international.org/ecma-262/5.1/#sec-15.10.2.6. While an identifier can contain $ and other unicode characters.

hyphenateOld('$Naughty') gets $-naughty, the new one gets $naughty.

But using dollar sign is not recommended in Vue. I think this change should be good.

@sliwey-zz
Copy link
Contributor Author

@HerringtonDarkholme
Thanks for your reminder. You're right, the new one can be keep consistent with the old when the parameter belongs to \w (it only contains a-zA-Z0-9_)

But I'm not sure if the parameter should contain $ or other unicode characters, so it may need to discuss whether this change is necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants