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

Move argument length requirement checks to a shared function to dedupe code #1563

Merged
merged 1 commit into from Jan 4, 2020

Conversation

levibuzolic
Copy link
Contributor

@levibuzolic levibuzolic commented Dec 11, 2019

I noticed in a compiled bundle that there were a lot of duplicated strings which were used for argument length validation, so I was interested to see what effect deduplicating these would have on bundle size.

Bundle Bytes
Before 25474
After 24528

That's a reduction of 946 bytes (3.71%) from a complete minified bundle.

@kossnocorp
Copy link
Member

I like it very much. Thank you for such great work! I considered making this change but was afraid that arguments leaking could affect the performance. I've run benchmarks, and it turned out that it's not a problem anymore.

I have a question, why did you choose to create a function before calling it inside of requiredArgs? I think that's redundant and unless there's a reason for this, I would ask you to change the API to simple requiredArgs(2, arguments)

@levibuzolic
Copy link
Contributor Author

@kossnocorp haha good point, originally I was planning to reuse instances but never came across for a need for it.

Will simplify and update!

@kossnocorp
Copy link
Member

Thank you so much! I'll ship it with the next release once you update it.

…e code.

I noticed in a compiled bundle that there were a lot of duplicated strings which were used for argument length validation, so I was interested to see what effect deduplicating these would have on bundle size.

Bundle | Bytes
-------|--------
Before | `25474`
Ater   | `24528`

That's a reduction of 946 bytes (3.71%) from a complete minified bundle.
@levibuzolic
Copy link
Contributor Author

@kossnocorp updated, thanks for the feedback. 👍

Copy link
Member

@kossnocorp kossnocorp left a comment

Choose a reason for hiding this comment

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

Amazing, thank you a lot!

@kossnocorp kossnocorp merged commit 16a561d into date-fns:master Jan 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants