-
-
Notifications
You must be signed in to change notification settings - Fork 8.9k
fix(provide): warn when using provide
after mounting
#13954
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
base: main
Are you sure you want to change the base?
Conversation
WalkthroughAdds warning tests for misuse of provide/inject and updates provide() runtime guards to warn when called without a current instance or after mount. Control flow in apiInject.ts is adjusted to use early guards and proceed with provides updates only when a current instance exists. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant CompInstance as ComponentInstance
participant Runtime as provide()
participant DI as Provides Map
Caller->>Runtime: provide(key, value)
Runtime->>CompInstance: get currentInstance
alt No currentInstance
note over Runtime: __DEV__ warn: "provide() can only be used inside setup."
Runtime-->>Caller: return (no mutation)
else Has currentInstance
alt Instance is mounted
note over Runtime: __DEV__ warn: "provide() should not be called after mount."
%% Proceed despite warning
end
Runtime->>DI: ensure current provides (possibly Object.create(parent))
Runtime->>DI: set key = value
Runtime-->>Caller: done
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧬 Code graph analysis (2)packages/runtime-core/__tests__/apiInject.spec.ts (2)
packages/runtime-core/src/apiInject.ts (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Size ReportBundles
Usages
|
@vue/compiler-core
@vue/compiler-dom
@vue/compiler-sfc
@vue/compiler-ssr
@vue/reactivity
@vue/runtime-core
@vue/runtime-dom
@vue/server-renderer
@vue/shared
vue
@vue/compat
commit: |
Closes #13921.
Background
As far as I'm aware,
provide
is only intended to be used during the creation of a component. The provided values are expected to be locked in once the component renders. Any descendants shouldn't need to worry about timing, any provided values will be ready immediately and won't change. This is baked into the design ofprovide
/inject
.I believe the original intention was for
provide
to only work directly insidesetup
(both the warning message and the documentation suggest this too). But, in practice, it doesn't check for that, it only checks for the availability ofcurrentInstance
. That is available in other places, such as inside lifecycle hooks.As pointed out in #13921, calling
provide
inside a lifecycle hook does mostly work, but not completely. Some parts of the code appear to have been written under the assumption that the provided values are only set prior to mounting. In my opinion, this is a valid assumption and the only change that is required is to tighten up the warnings to avoid incorrect usage.I've extended the warning to also check for
currentInstance.isMounted
. This will lead to a warning in cases like this:Why
isMounted
?I chose to use
currentInstance.isMounted
for my check because I think it reflects the underlying restriction. You could, for example, still useprovide
insideonBeforeMounted
. I'm not sure why that would be useful, but so long as the component hasn't mounted yet I didn't see any reason to log a warning.Impact on existing applications
This PR only adds extra warnings, it shouldn't change how
provide
behaves. If someone is usingprovide
insideonMounted
then it will still 'work' as it did previously, just with an extra warning. In that sense, it's relatively safe to merge and shouldn't impact production builds.I've marked this change as a
fix
because I think it needs to be included in the changelog. There will likely be some people who encounter this warning after upgrading and it would be helpful for them to be able to find this PR.I'm not sure what the use case for calling
provide
inside a lifecycle hook would be. It would still need to be called during the synchronous execution of the hook, so it isn't really suited to asynchronous tasks such as loading data. Perhaps there's a use case involving providing values that come from template refs?In any case, if
provide
is being used insideonMounted
then I believe the 'correct' way to handle that would be to provide a ref during setup instead, then update that ref from insideonMounted
.I'm not sure whether this would impact any libraries. I did have a quick look through VueUse. From what I could tell it doesn't use
provide
andinject
much, so it shouldn't be a problem.Tests
The existing warning message wasn't being tested. Most of the test cases I've added are testing the existing code on
main
, so even if the other changes in this PR aren't viable it may still be possible to merge some of those tests. Only the final test is actually testing my change toprovide
.Summary by CodeRabbit
Bug Fixes
Tests