Skip to content

Conversation

@edison1105
Copy link
Member

@edison1105 edison1105 commented Nov 20, 2025

close #14119

Summary by CodeRabbit

  • Bug Fixes

    • Improved consistency and correctness in handling readonly reactive arrays, ensuring proper identity preservation and reactivity during iteration and element manipulation.
  • Tests

    • Added test coverage for readonly ref-wrapped array behavior.

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

@coderabbitai
Copy link

coderabbitai bot commented Nov 20, 2025

Walkthrough

Fixed a bug where deep readonly semantics were not preserved when iterating over readonly ref-wrapped arrays. Introduced a new toWrapped helper to unify item wrapping behavior across reactive, readonly, and shallow variants, and updated array instrumentation methods to consistently apply correct wrapping semantics.

Changes

Cohort / File(s) Summary
Test Coverage
packages/reactivity/__tests__/readonly.spec.ts
Adds test case verifying that element identity and readonly characteristics are preserved when iterating over readonly ref-wrapped arrays.
Reactivity Core Logic
packages/reactivity/src/arrayInstrumentations.ts
Introduces toWrapped helper to determine correct value wrapping (toReactive/toReadonly) based on target state. Updates array iteration methods (forEach, map, filter, find, reduce, etc.) to use toWrapped instead of direct toReactive calls. Extends public imports to include isReactive, isReadonly, isShallow, and toReadonly.

Sequence Diagram

sequenceDiagram
    participant Code as Calling Code
    participant Method as Array Method<br/>(e.g., forEach, map)
    participant toWrapped as toWrapped Helper
    participant Target as Target Proxy State
    
    Code->>Method: Invoke iteration method on readonly array
    Method->>toWrapped: For each element, determine wrapping
    toWrapped->>Target: Check: isReactive? isReadonly?<br/>isShallow?
    Target-->>toWrapped: Target state flags
    alt Readonly target
        toWrapped-->>Method: Apply toReadonly
    else Reactive (non-shallow) target
        toWrapped-->>Method: Apply toReactive
    else Shallow or other
        toWrapped-->>Method: Return unwrapped value
    end
    Method-->>Code: Return element with correct wrapper
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Core logic involves a new toWrapped helper that must correctly dispatch wrapping decisions across multiple conditions (reactive, readonly, shallow states)
  • Updates span multiple array instrumentation methods (forEach, map, filter, find, findLast, values, reduce variants), requiring verification that each call site is consistent
  • New test case should be verified to capture the original issue and validate the fix

Possibly related PRs

Suggested labels

scope: reactivity, :hammer: p3-minor-bug

Poem

🐰 Through arrays we hop, both deep and with care,
Readonly wraps spread their semantics so fair,
No more do the elements their nature betray,
When mapped, filtered, found—they stay the right way! ✨

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately and concisely describes the main fix: correctly wrapping iterated array items to preserve readonly status.
Linked Issues check ✅ Passed The PR addresses issue #14119 by introducing toWrapped logic to ensure iteration methods return readonly-wrapped items matching index-based access behavior.
Out of Scope Changes check ✅ Passed All changes are scoped to fixing readonly preservation in array iteration: test addition, new toWrapped wrapper logic, and array instrumentation updates.
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
  • Commit unit tests in branch edison/fix/14119

📜 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 83f6ab6 and 6fca482.

📒 Files selected for processing (2)
  • packages/reactivity/__tests__/readonly.spec.ts (1 hunks)
  • packages/reactivity/src/arrayInstrumentations.ts (8 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: naramdash
Repo: vuejs/core PR: 14058
File: packages/reactivity/src/collectionHandlers.ts:252-271
Timestamp: 2025-11-05T14:53:01.052Z
Learning: In Vue's reactivity system, collection methods that create new collections (like Array's slice(), concat(), filter() and Set's difference(), intersection(), union(), symmetricDifference()) intentionally return raw (non-reactive) collections, not reactive proxies. This is by design - only the values/elements inside can remain reactive if they were originally reactive.
📚 Learning: 2025-11-05T14:53:01.052Z
Learnt from: naramdash
Repo: vuejs/core PR: 14058
File: packages/reactivity/src/collectionHandlers.ts:252-271
Timestamp: 2025-11-05T14:53:01.052Z
Learning: In Vue's reactivity system, collection methods that create new collections (like Array's slice(), concat(), filter() and Set's difference(), intersection(), union(), symmetricDifference()) intentionally return raw (non-reactive) collections, not reactive proxies. This is by design - only the values/elements inside can remain reactive if they were originally reactive.

Applied to files:

  • packages/reactivity/src/arrayInstrumentations.ts
🧬 Code graph analysis (2)
packages/reactivity/src/arrayInstrumentations.ts (1)
packages/reactivity/src/index.ts (4)
  • isReadonly (27-27)
  • isReactive (26-26)
  • toReadonly (35-35)
  • toReactive (34-34)
packages/reactivity/__tests__/readonly.spec.ts (3)
packages/reactivity/src/index.ts (5)
  • readonly (25-25)
  • ref (2-2)
  • computed (44-44)
  • isReadonly (27-27)
  • isReactive (26-26)
packages/reactivity/src/ref.ts (1)
  • ref (59-61)
packages/reactivity/src/computed.ts (1)
  • computed (197-220)
⏰ 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)
  • GitHub Check: Redirect rules
  • GitHub Check: Header rules
  • GitHub Check: Pages changed
🔇 Additional comments (6)
packages/reactivity/__tests__/readonly.spec.ts (1)

176-187: LGTM! Excellent test coverage for the readonly iteration fix.

This test directly validates the fix for issue #14119 by ensuring that:

  1. Elements accessed via forEach maintain identity with index-accessed elements
  2. Deep readonly status is preserved during iteration
  3. Reactive characteristics are maintained when readonly wraps reactive sources

The test scenario accurately reproduces the reported bug and confirms the fix works correctly.

packages/reactivity/src/arrayInstrumentations.ts (5)

3-11: LGTM! Imports properly support the new wrapping logic.

The added imports (isReactive, isReadonly, toReadonly) are all utilized by the new toWrapped helper to determine correct wrapping semantics based on the target's reactive and readonly status.


35-40: Excellent design! The unified wrapping helper correctly preserves readonly and reactive semantics.

The toWrapped function elegantly handles three scenarios:

  1. Readonly + Reactive (e.g., readonly(reactive([...]))): Wraps items as both
  2. Readonly only (e.g., readonly([...])): Wraps items as readonly
  3. Reactive only (e.g., reactive([...])): Wraps items as reactive

This ensures that array iteration methods preserve the same proxy characteristics as index-based access, fixing the core issue reported in #14119.


46-46: LGTM! Iterator methods now correctly preserve readonly wrapping.

The changes to Symbol.iterator, entries, and values ensure that iteration-based access returns items with the same wrapping as index-based access. Passing this (the proxy array) to toWrapped correctly captures the target's readonly and reactive characteristics.

Also applies to: 57-58, 228-228


73-80: LGTM! Higher-order methods correctly wrap return values.

The filter, find, and findLast methods now use toWrapped in their return value wrappers:

  • filter: Wraps each item in the returned array
  • find/findLast: Wraps the returned item

This ensures that return values maintain the correct readonly and reactive characteristics of the source array.

Also applies to: 87-94, 108-115


296-296: LGTM! Internal helpers now consistently apply correct wrapping.

The updates to apply and reduce helpers ensure that all array methods using these helpers (forEach, map, every, some, reduce, reduceRight, etc.) pass correctly-wrapped items to user callbacks. This provides comprehensive coverage across all iteration-based array methods.

Also applies to: 320-320


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

@github-actions
Copy link

Size Report

Bundles

File Size Gzip Brotli
runtime-dom.global.prod.js 103 kB (+122 B) 38.9 kB (+25 B) 35.1 kB (+53 B)
vue.global.prod.js 161 kB (+122 B) 58.9 kB (+31 B) 52.4 kB (+63 B)

Usages

Name Size Gzip Brotli
createApp (CAPI only) 47 kB (+126 B) 18.3 kB (+29 B) 16.8 kB (+26 B)
createApp 55.2 kB (+122 B) 21.4 kB (+42 B) 19.6 kB (+22 B)
createSSRApp 59.4 kB (+122 B) 23.2 kB (+43 B) 21.1 kB (+40 B)
defineCustomElement 60.8 kB (+122 B) 23.2 kB (+25 B) 21.1 kB (+40 B)
overall 69.5 kB (+122 B) 26.7 kB (+32 B) 24.3 kB (-18 B)

@pkg-pr-new
Copy link

pkg-pr-new bot commented Nov 20, 2025

Open in StackBlitz

@vue/compiler-core

npm i https://pkg.pr.new/@vue/compiler-core@14120

@vue/compiler-dom

npm i https://pkg.pr.new/@vue/compiler-dom@14120

@vue/compiler-sfc

npm i https://pkg.pr.new/@vue/compiler-sfc@14120

@vue/compiler-ssr

npm i https://pkg.pr.new/@vue/compiler-ssr@14120

@vue/reactivity

npm i https://pkg.pr.new/@vue/reactivity@14120

@vue/runtime-core

npm i https://pkg.pr.new/@vue/runtime-core@14120

@vue/runtime-dom

npm i https://pkg.pr.new/@vue/runtime-dom@14120

@vue/server-renderer

npm i https://pkg.pr.new/@vue/server-renderer@14120

@vue/shared

npm i https://pkg.pr.new/@vue/shared@14120

vue

npm i https://pkg.pr.new/vue@14120

@vue/compat

npm i https://pkg.pr.new/@vue/compat@14120

commit: 6fca482

@edison1105
Copy link
Member Author

/ecosystem-ci run

@vue-bot
Copy link
Contributor

vue-bot commented Nov 20, 2025

📝 Ran ecosystem CI: Open

suite result latest scheduled
language-tools success success
radix-vue success success
primevue success success
pinia success success
test-utils success success
vite-plugin-vue success success
quasar success success
vuetify success success
router failure failure
vitepress success success
vant success failure
vue-i18n failure failure
vue-macros failure failure
vueuse success success
nuxt failure failure
vue-simple-compiler success success

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.

Deep readonly is not preserved through array iteration methods

3 participants