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(v2): replace unnecessary json stringify(string) with inline string #2039

Merged
merged 1 commit into from
Nov 24, 2019

Conversation

endiliey
Copy link
Contributor

Motivation

Rather than doing stringify for simple string like below

const a = `test${JSON.stringify(word)}`;

Better do

const a = `test"${word}"`;

image

https://jsperf.com/stringifyvsdirect/1

Have you read the Contributing Guidelines on pull requests?

yes

Test Plan

(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work. Bonus points for screenshots and videos!)

  • Locally everything still ok
  • Netlify
  • CI pass

@endiliey endiliey added the pr: performance This PR does not add a new behavior, but existing behaviors will be more memory- / time-efficient. label Nov 23, 2019
@endiliey endiliey requested a review from yangshun as a code owner November 23, 2019 14:44
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Nov 23, 2019
@docusaurus-bot
Copy link
Contributor

Deploy preview for docusaurus-2 ready!

Built with commit fd142fa

https://deploy-preview-2039--docusaurus-2.netlify.com

@docusaurus-bot
Copy link
Contributor

Deploy preview for docusaurus-preview ready!

Built with commit fd142fa

https://deploy-preview-2039--docusaurus-preview.netlify.com

Copy link
Contributor

@yangshun yangshun left a comment

Choose a reason for hiding this comment

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

Good catch. I wonder why we even wrote that in the first place.

@yangshun yangshun merged commit 3aa2ab6 into master Nov 24, 2019
@yangshun yangshun deleted the endi/jsonstringifyremove branch November 24, 2019 02:24
@endiliey
Copy link
Contributor Author

previously it wasnt a string. Its an object iirc. Some refactor did previously and sometimes miss this small detail

@endiliey
Copy link
Contributor Author

endiliey commented Dec 4, 2019

Seems like regression. Need to revert this. Turns out I put JSON.stringify previously to escape backslash on windows.

Should've put comment lol..

@yangshun
Copy link
Contributor

yangshun commented Dec 4, 2019

Haha we should have also went to see the commit in the past to see why we added that (isn't always useful, but could be useful). Feel free to revert.

@endiliey
Copy link
Contributor Author

endiliey commented Dec 4, 2019

Doing it tonight. I am quite occupied this week with personal stuff (eg: renewing my passport)

endiliey added a commit that referenced this pull request Dec 5, 2019
endiliey added a commit that referenced this pull request Dec 5, 2019
* Revert "perf(v2): replace unnecessary json stringify(string) with inline string (#2039)"

This reverts commit 3aa2ab6.

* add comment

* Comment bois
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: performance This PR does not add a new behavior, but existing behaviors will be more memory- / time-efficient.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants