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

fix(runtime-core): app.provide throws a warning when assigning the same value. #10337

Closed
wants to merge 3 commits into from

Conversation

OnlyWick
Copy link
Contributor

@OnlyWick OnlyWick commented Feb 13, 2024

If providing the same value to app.provide, no warning should be thrown.

Copy link

github-actions bot commented Feb 13, 2024

Size Report

Bundles

File Size Gzip Brotli
runtime-dom.global.prod.js 90.4 kB (+22 B) 34.4 kB (+14 B) 31 kB (+20 B)
vue.global.prod.js 148 kB (+22 B) 53.7 kB (+15 B) 47.9 kB (-7 B)

Usages

Name Size Gzip Brotli
createApp 50.6 kB (+36 B) 19.7 kB (+18 B) 18 kB (+9 B)
createSSRApp 53.9 kB (+36 B) 21.1 kB (+12 B) 19.2 kB (+15 B)
defineCustomElement 52.9 kB (+36 B) 20.5 kB (+18 B) 18.7 kB (+14 B)
overall 64.3 kB (+36 B) 24.8 kB (+11 B) 22.5 kB (+4 B)

@OnlyWick OnlyWick changed the title feat(runtime-core): allow provide reassignment fix(runtime-core): allow provide reassignment Feb 13, 2024
@OnlyWick OnlyWick changed the title fix(runtime-core): allow provide reassignment fix(runtime-core): app.provide throws a warning when assigning the same value. Feb 15, 2024
warn(
`App already provides property with key "${String(key)}". ` +
`It will be overwritten with the new value.`,
)
}

context.provides[key as string | symbol] = value
if (isValueChanged) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (isValueChanged) { could be merged with previous if

Copy link
Contributor Author

@OnlyWick OnlyWick Feb 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you suggesting to put it inside an else block? Since the value is already the same, there is no need to perform the assignment again.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no

if (isValueChanged) {
    if (...) warn()
  
    context.provides[key as string | symbol] = newVal
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it.

@OnlyWick
Copy link
Contributor Author

Perhaps the warning needs to be optimized? I think if the same value is assigned, it should only warn the user that the same value already exists, instead of warning the user about overwriting the old value with a new one.

@haoqunjiang haoqunjiang added the 🧹 p1-chore Priority 1: this doesn't change code behavior. label Feb 22, 2024
@yyx990803
Copy link
Member

I don't think this is necessary. Providing the same value twice is pointless and should also be avoided if possible so a warning is appropriate.

@yyx990803 yyx990803 closed this Feb 25, 2024
@OnlyWick OnlyWick deleted the allow-provide-reassignment branch February 25, 2024 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🧹 p1-chore Priority 1: this doesn't change code behavior.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants