refactor(reactivity): add __NO_SIDE_EFFECTS__ comments#14308
refactor(reactivity): add __NO_SIDE_EFFECTS__ comments#14308edison1105 merged 2 commits intovuejs:mainfrom
__NO_SIDE_EFFECTS__ comments#14308Conversation
📝 WalkthroughWalkthroughAdds Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/reactivity/src/ref.ts (1)
58-64: Consider adding annotation toref()as well.The
ref()function follows the same pattern asshallowRef()(which is annotated)—it delegates tocreateRef()and returns aRefImpl. For consistency, it could also receive the/*@__NO_SIDE_EFFECTS__*/annotation.Suggested change
export function ref<T = any>(): Ref<T | undefined> +/*@__NO_SIDE_EFFECTS__*/ export function ref(value?: unknown) { return createRef(value, false) }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/reactivity/src/computed.tspackages/reactivity/src/reactive.tspackages/reactivity/src/ref.ts
⏰ 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). (1)
- GitHub Check: test / e2e-test
🔇 Additional comments (8)
packages/reactivity/src/reactive.ts (3)
91-105: Annotation placement looks correct for TypeScript overloads.Placing
/*@__NO_SIDE_EFFECTS__*/between the overload signature and implementation ensures it appears directly before the function in compiled JavaScript output (since overloads are erased). Thereactive()function stores the proxy in a WeakMap, but this is not an observable side effect for tree-shaking purposes—if the return value is unused, the call can be safely eliminated.Since this is a WIP and you're planning to test with
pkg-pr-new, verify the annotations are preserved through Vue's build pipeline and that bundlers (esbuild, Rollup, Terser) correctly tree-shake unused calls.
141-152: LGTM for factory function annotations.
shallowReactive,readonly, andshallowReadonlyfollow the same pattern asreactive—they create proxies stored in WeakMaps but have no external observable side effects.Also applies to: 207-218, 250-259
322-328: LGTM for predicate and utility function annotations.
isReactive,isReadonly,isShallow,isProxy, andtoRaware pure functions that only read properties without mutation—ideal candidates for/*@__NO_SIDE_EFFECTS__*/.Also applies to: 341-344, 346-349, 358-361, 386-390
packages/reactivity/src/ref.ts (4)
45-49: LGTM forisRefannotation.Pure predicate function that only checks a flag property.
97-100: LGTM forshallowRefannotation.Creates a
RefImplinstance without external side effects.
343-353: LGTM fortoRefsannotation.The dev-mode warning is acceptable for tree-shaking purposes since
__DEV__is typically stripped in production builds.
480-495: LGTM fortoRefannotation.All branches either return existing refs or create new ref objects without external side effects.
packages/reactivity/src/computed.ts (1)
197-221: LGTM forcomputedannotation.The
computed()function creates aComputedRefImplwhose constructor only initializes internal state. The dev-only debug callback assignments don't affect tree-shaking in production builds.
Found a list here: https://github.com/javascript-compiler-hints/compiler-notations-spec/blob/main/no-side-effects-notation-compatibility.md |
3 participants
This PR adds
/*@__NO_SIDE_EFFECTS__*/for the following functions:computedreactiveshallowReactivereadonlyshallowReadonlyisReactiveisReadonlyisShallowisProxytoRawisRefrefshallowReftoRefstoRefThis allows for better tree-shaking, though the benefit is only likely to be small in most cases.
The motivation here comes from some code in Pinia:
This initial creation is not inside a
__DEV__check (for scoping reasons), but all other uses ofhotStateare. The variablehotStateis removed from production builds, but the call toref({})is retained.Adding
/*@__NO_SIDE_EFFECTS__*/should allow these function calls to be removed in use cases like this, without needing the consuming application/library to add/*#__PURE__*/annotations.I'm not sure exactly which build tools support
/*@__NO_SIDE_EFFECTS__*/, but I tested usingpkg-pr-newfor this PR and a defaultcreate-vueproject and it did seem to work there.I think the biggest risk here is if I've incorrectly annotated a function. Some of these functions do have side-effects, such as populating the
proxyMapincreateReactiveObject, but I believe those side-effects can be ignored if the return value is unused.I should also note that accessing a property of the return value is enough to prevent the code being removed. So something like
computed(() => {})will be removed, butcomputed(() => {}).valuewill not.I'm not sure how to classify this change. I've gone with
refactor, but I can see an argument for eitherfixorperf.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.