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

Memory usage #4

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Memory usage #4

wants to merge 4 commits into from

Conversation

Zibx
Copy link

@Zibx Zibx commented Feb 26, 2019

I want to use your library in high speed env, so I am reviewing all foreign code.

You do not need to create new translate function instance on each relativeDate call.
Constants can also be created once.

You do not need to create new translate function instance on each relativeDate call.
Constants can also be created once.
Now it should build and pass tests
I have teached this lesson. Test everything locally. If this build fail - I would download repo and fix everything locally.
@wildlyinaccurate
Copy link
Owner

I love your final commit message!

These changes seem fine to me. I would prefer that the translate() function didn't use a value that is modified in a different block scope, but I think the code is simple enough that it won't cause any maintenance nightmares.

Out of curiosity: do you have any benchmarks comparing this library before and after your change?

@wildlyinaccurate
Copy link
Owner

I just want to test this out on a couple of projects before I merge. I've been extra-cautious about releasing updates to this package since it became a dependency of npm.

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.

2 participants