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

Avoid expensive formatting for simple messages #233

Merged
merged 1 commit into from
Dec 1, 2015

Conversation

ericf
Copy link
Collaborator

@ericf ericf commented Dec 1, 2015

This avoid expensive message formatting for simple messages without values. In development messages will always be formatted in case of missing values that are suppose to be there.

This should drastically improve performance, both memory and CPU, for all apps.

@kfay
Copy link

kfay commented Dec 1, 2015

This is going to help a ton! Thanks @ericf. 👍

@caridy
Copy link
Collaborator

caridy commented Dec 1, 2015

looks good but maybe we should just wire this up at the beginning of the method, not need to skip a bunch of conditions if no values are provided, and we are in prod. from the current diff it is difficult to imagine that this is a short circuit process, you have to read the whole thing to encounter that it reality it is just falling back to message.

This avoid expensive message formatting for simple messages without
`values`. In development messages will always be formatted in case of
missing `values` that are suppose to be there.
@ericf
Copy link
Collaborator Author

ericf commented Dec 1, 2015

@caridy yeah the early return makes the diff much easier to read. I updated to use that instead.

@caridy
Copy link
Collaborator

caridy commented Dec 1, 2015

🎱

ericf added a commit that referenced this pull request Dec 1, 2015
Avoid expensive formatting for simple messages
@ericf ericf merged commit ebc34e2 into formatjs:master Dec 1, 2015
@ericf ericf deleted the simple-messages branch December 1, 2015 23:43
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.

None yet

3 participants