Skip to content

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

Merged
kossnocorp merged 1 commit into
date-fns:masterfrom
levibuzolic:required-args-optimisation
Jan 4, 2020
Merged

kossnocorp merged 1 commit into
date-fns:masterfrom
levibuzolic:required-args-optimisation

Conversation

@levibuzolic

@levibuzolic levibuzolic commented Dec 11, 2019 •

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
levibuzolic force-pushed the required-args-optimisation branch from b647987 to bc1a023 Compare January 4, 2020 00:26
@levibuzolic

Copy link
Copy Markdown
Contributor Author

@kossnocorp updated, thanks for the feedback. 👍

@kossnocorp kossnocorp left a comment

Copy link
Copy Markdown
Member

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.

2 participants