Skip to content

Conversation

@micaiguai
Copy link

@micaiguai micaiguai commented Dec 10, 2025

[ReactiveFlags.IS_SHALLOW] is declared with a default value of false, but the constructor always overwrites it based on the isShallow parameter, making the default dead code.

Removing it cleans up the code and avoids the misleading impression that [ReactiveFlags.IS_SHALLOW] default to false.

First time contributing to this project, thanks for reviewing, any feedback welcome!

Summary by CodeRabbit

  • Refactor
    • Internal code structure improvements to the reactivity system with no end-user visible changes.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Dec 10, 2025

Walkthrough

The initialization of the [ReactiveFlags.IS_SHALLOW] field in the RefImpl class was moved from the field declaration to the constructor, preserving the boolean type and deferred initialization semantics.

Changes

Cohort / File(s) Summary
RefImpl field initialization refactoring
packages/reactivity/src/ref.ts
Removed inline initializer (= false) from [ReactiveFlags.IS_SHALLOW] readonly field declaration; assignment deferred to constructor while maintaining boolean type and false default value

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

  • Field initialization timing change with no functional impact; verify constructor properly initializes the field to false

Poem

🐰 A tiny hop in timing's dance,
From field to constructor's chance,
IS_SHALLOW still plays its part,
Just deferred with clever art!
Same semantics, different score—
Clean and tidy, nothing more! ✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: removing a redundant default initialization of IS_SHALLOW in RefImpl that was always overwritten by the constructor.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f51d3e2 and b20bfb7.

📒 Files selected for processing (1)
  • packages/reactivity/src/ref.ts (1 hunks)
🔇 Additional comments (1)
packages/reactivity/src/ref.ts (1)

116-123: Removing the redundant IS_SHALLOW default looks correct and keeps semantics unchanged

RefImpl’s [ReactiveFlags.IS_SHALLOW] is always set from the isShallow constructor parameter (see Line 122) and all call sites to new RefImpl pass an explicit boolean via createRef(rawValue, shallow), so dropping the = false initializer simply removes a redundant write and avoids implying any meaningful default. Runtime behavior after construction is unchanged.

One thing to double‑check is TypeScript’s strictPropertyInitialization with a computed property name: ensure your current TS config and version are happy with public readonly [ReactiveFlags.IS_SHALLOW]: boolean being initialized only via this[ReactiveFlags.IS_SHALLOW] = isShallow in the constructor. If any TS error appears, the usual fallback would be to either keep a non‑misleading initializer or add a definite‑assignment assertion (!: boolean) instead of a bare type. Otherwise, this cleanup is good as‑is.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@edison1105
Copy link
Member

Thanks for your contribution! However, per our contributing guidelines:

It should be noted that we discourage contributors from submitting code refactors that are largely stylistic. Code refactors are only accepted if it improves performance, or comes with sufficient explanations on why it objectively improves the code quality (e.g. makes a related feature implementation easier).

This change falls into the category of stylistic refactoring. While the default value = false is overwritten in the constructor, it serves as an explicit documentation of the property's default semantic meaning. Removing it doesn't provide objective improvements in performance or code quality - it's more of a subjective preference.

We appreciate your interest in improving Vue, but we'd like to focus PR reviews on changes that have more substantial impact. Feel free to contribute bug fixes or features that address real user needs!

@edison1105 edison1105 closed this Dec 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants