Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Thanks for PR 👍
Could you tell me how this change will improve performance when
_vm.messages[locale]
is undefined or empty object ?This change introduce
Object.keys
, which seems to add extra execution time.I also assume
_vm.messages[locale]
is likely to have several messages for many cases, so this change will degrade performance for many cases.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.
Hi exoego,
vue-i18n/src/index.js
Line 769 in 4b34aa9
With reference to the current logic, the
merge
call becomes eithermerge({}, this._vm.messages[locale], message)
ormerge({}, {}, message)
when_vm.messages[locale]
is defined or undefined respectively. Themerge
function loops the messages twice in both cases.With this change, the messages will only be looped once.
Below simple test gives me the result as follows. The time difference will be greater when there are more messages.
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.
Thanks for explanation and quick benchmark.
Yes, this is exactly the case I am afraid in the previous comment:
_vm.messages[locale]
is likely to have several messages for many cases, so this change will degrade performance for many cases.Could you tell more detail of your use case #1099, where
mergeLocaleMessage
is invoked over undefined or empty message ?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.
Thanks for your review.
Sorry for my vague statement, I meant the time difference (before and after the change) will be greater. So the improvement of this pull request will be greater when there are more messages to merge which should be the usual cases.
If I replace
{ foo: 'bar' }
in above benchmark with an object of 10 keys. The result becomes:I just tried to improve the performance without logic change. The use case was introduced in #752 by another person and was released in version 8.15.3.
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.
@cslee
Thank you for your reporting!
I actually tried to check about performance.
The more locale messages, the more noticeable the performance degradation becomes.
This is a problem.
You can solve the performance degradation with PR.
Thanks! :)
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.
This change can let watch failed with Vue2.