-
-
Notifications
You must be signed in to change notification settings - Fork 119
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
Stringify object using Object.entries instead of separated function #418
base: master
Are you sure you want to change the base?
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/cristianbote/goober-rocks/4x3LCsWFPTbu2ZE9MDfS13WmSs16 |
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit b8c8503:
|
Thanks @RealPeha! Will look into the perf to see if there are any regressions. |
Replaced |
export { extractCss } from './core/update'; | ||
export { styled, setup } from './styled'; | ||
export { css, glob, keyframes } from './css'; |
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 this is generated by microbundle but I've added a flag to not get this anymore. Can you check that you have this line in your fork? https://github.com/cristianbote/goober/blob/master/package.json#L75
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.
Yes, I have the same package.json, but this file is still being created
@RealPeha thank you so much for your work on this! 🙏 I appreciate it! Looked at the perf scores and this is the breakdown: For node env:
Changed locally the test so it'll head-to-head the changed version -- the one with Object.entries, with the regular master one. This is a pretty decent regression in terms of perf. This will affect SSR performance or build durations. Now in terms of browser:
There really isn't much of a difference. What are your thoughts on this? On one hand goober does get smaller but at the cost of perf degradation on node env and on the other the browser based rendering is just fine. |
I think that performance should be a priority, and reducing the bundle by a couple of bytes does not provide any benefit. In any case, I'm doing this for fun and I have a more ideas on how to shave off a couple of bytes, so there will be new PRs soon 😉 |
Shaved off quite a few bytes by removing the
stringify
function and usingObject.entries
instead. I don’t know how this affected performance, running the benchmark did not give me the opportunity to evaluate the difference.Also now this stringify returns a slightly different result.
Example object:
Old version:
value10
My version:
value,10
This affected the hash, so I had to edit the tests
Size comparison
Before:After: