fix: messages deepCopy mutates src arguments

[BobbieGoede]

Important

This has not been thoroughly tested but should demonstrate the issue with deepCopy

Currently during deepCopy if a key is unset, it will be assigned to the value of the src[key], this is an issue for objects as they will be assigned by reference, meaning following calls to deepCopy will assign properties on these objects.

So far a simple solution seems to be to set the property value to an empty object/array, this ensures both src[key] and des[key] are objects meaning deepCopy will merge these as such instead of direct assignment.

I've had this saved in a git stash for some time so it has been a while since debugging this issue, I'm opening this PR so I won't forget about it again 😅

nuxt-modules/i18n#3089
nuxt-modules/i18n#2803

[pkg-pr-new]

Open in Stackblitz

@intlify/core

pnpm add https://pkg.pr.new/@intlify/core@1947

@intlify/devtools-types

pnpm add https://pkg.pr.new/@intlify/devtools-types@1947

@intlify/core-base

pnpm add https://pkg.pr.new/@intlify/core-base@1947

@intlify/message-compiler

pnpm add https://pkg.pr.new/@intlify/message-compiler@1947

@intlify/shared

pnpm add https://pkg.pr.new/@intlify/shared@1947

petite-vue-i18n

pnpm add https://pkg.pr.new/petite-vue-i18n@1947

vue-i18n

pnpm add https://pkg.pr.new/vue-i18n@1947

@intlify/vue-i18n-core

pnpm add https://pkg.pr.new/@intlify/vue-i18n-core@1947

commit: 05b2353

[kazupon]

Looks good to me!
Thank you!

[BobbieGoede]

I'm curious what the impact of this change is on performance since previously the objects assigned by reference were not looped over by key.

[kazupon]

Yeah, performance is certainly a concern, and I need to look at the cost of isObject checks and object key refer in the if condition.
It's worth doing a simple performance benchmark using deepCopy.

[BobbieGoede]

Somewhat related to performance and object checks (but not really related to this PR), I feel like #1379 should be reverted.

The change was made to support Safari 10 which is not receiving security updates (Apple doesn't issue EOL dates), the original code seems to work fine for Vue core https://github.com/vuejs/core/blob/main/packages/shared/src/general.ts#L74-L75 and is likely more performant as well.

[kazupon]

Somewhat related to performance and object checks (but not really related to this PR), I feel like #1379 should be reverted.

Let's do it!

[BobbieGoede]

Yeah, performance is certainly a concern, and I need to look at the cost of isObject checks and object key refer in the if condition. It's worth doing a simple performance benchmark using deepCopy.

I tried bench testing the change and it looks like it does impact performance but not excessively so, it's worth the trade-off. The way I tested it is not pretty so I pushed it to a separate branch, you can check out the changes and results in this PR BobbieGoede#5, I'm assuming we don't want to add a bench like this.

[kazupon]

Thanks!
I've checked your benchmark.
I think the performance is still acceptable too.
I'll merge this PR!