fix(compat): fix compat handler of draggable by yangxiuxiu1115 · Pull Request #12445 · vuejs/core

yangxiuxiu1115 · 2024-11-20T10:19:20Z

Summary by CodeRabbit

Bug Fixes
- Improved handling of enumerated attributes so null, false, and the string "false" are coerced and rendered consistently.
Tests
- Added a test verifying attribute preservation and deprecation warnings when enumerated attribute coercion occurs under compatibility mode.

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

github-actions · 2024-11-20T10:20:16Z

Size Report

Bundles

File	Size	Gzip	Brotli
runtime-dom.global.prod.js	103 kB	38.9 kB	35.1 kB
vue.global.prod.js	161 kB	58.8 kB	52.4 kB

Usages

Name	Size	Gzip	Brotli
createApp (CAPI only)	46.9 kB	18.3 kB	16.8 kB
createApp	55 kB	21.4 kB	19.6 kB
createSSRApp	59.3 kB	23.1 kB	21.1 kB
defineCustomElement	60.6 kB	23.1 kB	21.1 kB
overall	69.3 kB	26.6 kB	24.3 kB

pkg-pr-new · 2024-11-20T10:20:33Z

Open in StackBlitz

@vue/compiler-core

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

@vue/compiler-dom

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

@vue/compiler-sfc

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

@vue/compiler-ssr

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

@vue/reactivity

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

@vue/runtime-core

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

@vue/runtime-dom

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

@vue/server-renderer

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

@vue/shared

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

vue

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

@vue/compat

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

commit: 102f560

edison1105 · 2024-11-21T01:37:42Z

/ecosystem-ci run

vue-bot · 2024-11-21T01:44:09Z

📝 Ran ecosystem CI: Open

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

skirtles-code · 2025-05-20T14:20:42Z

packages/vue-compat/__tests__/global.spec.ts

  expect(app2.config.globalProperties.test).toBe(undefined)
 })
+
+test('ATTR_ENUMERATED_COERCION: true', () => {


I'm wondering whether the tests for this should be in misc.spec.ts instead, as there are already tests for ATTR_ENUMERATED_COERCION in that file?

skirtles-code · 2025-05-20T14:35:19Z

packages/runtime-dom/src/modules/attrs.ts

+      value === null || value === false || value === 'false'
        ? 'false'
-        : typeof value !== 'boolean' && value !== undefined
+        : value !== undefined


I think the logic is correct, but it might be easier to understand if the undefined check were handled first?

coderabbitai · 2025-05-27T09:36:40Z

Walkthrough

Updated compat enumerated-attribute coercion so string "false", false, and null coerce to "false", undefined returns null, otherwise "true". Added a test verifying draggable="false" renders as "false" and emits a deprecation warning.

Changes

Cohort / File(s)	Change Summary
Runtime DOM attr logic `packages/runtime-dom/src/modules/attrs.ts`	Revised `compatCoerceAttr` internals: return `null` for `undefined`; treat `null`, `false`, and the string `'false'` as `'false'`; otherwise return `'true'`. No exported signatures changed.
Compat tests `packages/vue-compat/__tests__/misc.spec.ts`	Added `ATTR_ENUMERATED_COERCION` test mounting `draggable="false"` and `:spellcheck="false"`, asserting rendered attributes and deprecation warnings.

Sequence Diagram(s)

sequenceDiagram
    participant TestRunner
    participant VueCompat
    participant compatCoerceAttr
    participant DOM

    TestRunner->>VueCompat: mount template with ATTR_ENUMERATED_COERCION enabled
    VueCompat->>compatCoerceAttr: coerce attribute "draggable" with value "false"
    compatCoerceAttr-->>VueCompat: returns "false"
    VueCompat->>DOM: render element with draggable="false"
    VueCompat-->>TestRunner: emit deprecation warning(s)
    TestRunner->>TestRunner: assert DOM and warnings

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Pay attention to compatCoerceAttr conditional logic and edge cases (string vs boolean vs null/undefined).
Verify tests cover both literal attributes and bound props (e.g., "false" vs :spellcheck="false").

Possibly related PRs

fix(compat): ensure false value on input value retains attribute #13216 — Modifies the same compatCoerceAttr implementation in packages/runtime-dom/src/modules/attrs.ts, addressing how enumerated attributes are coerced.

Suggested reviewers

KazariEX

Poem

A rabbit hopped in code at night,
Turned wrong-to-right with gentle bite.
"false" stays false, no sneaky trick—
Tests applaud with pace so quick.
🥕

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 describes the main change: fixing the compat handler for the draggable attribute.
Linked Issues check	✅ Passed	The code changes directly address issue #12444 by fixing the enumerated attribute coercion logic so draggable="false" is preserved instead of coerced to true.
Out of Scope Changes check	✅ Passed	All changes are scoped to fixing the enumerated attribute coercion logic and adding a test case; no out-of-scope modifications detected.
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 5e0e684 and 102f560.

📒 Files selected for processing (1)

packages/vue-compat/__tests__/misc.spec.ts (1 hunks)

✅ Files skipped from review due to trivial changes (1)

packages/vue-compat/tests/misc.spec.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). (2)

GitHub Check: continuous-release
GitHub Check: autofix

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

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

packages/vue-compat/__tests__/misc.spec.ts (1)

287-289: Note: Pre-existing type annotation pattern (not blocking).

The static analysis tool flags the use of Function as a type, suggesting to explicitly define the function shape. However, this pattern is used consistently throughout the file (lines 262, 268, 274, etc.), so this is a pre-existing codebase pattern rather than an issue introduced by this change.

🧰 Tools

🪛 Biome (1.9.4)

[error] 288-288: Don't use 'Function' as a type.

Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.

(lint/complexity/noBannedTypes)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2a58947 and 5e0e684.

📒 Files selected for processing (2)

packages/runtime-dom/src/modules/attrs.ts (1 hunks)
packages/vue-compat/__tests__/misc.spec.ts (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

packages/runtime-dom/src/modules/attrs.ts

🧰 Additional context used

🪛 Biome (1.9.4)

packages/vue-compat/__tests__/misc.spec.ts

[error] 288-288: Don't use 'Function' as a type.

Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.

(lint/complexity/noBannedTypes)

⏰ Context from checks skipped due to timeout of 90000ms (2)

GitHub Check: test / e2e-test
GitHub Check: test / unit-test-windows

🔇 Additional comments (1)

packages/vue-compat/__tests__/misc.spec.ts (1)

279-291: LGTM! Good test coverage for the compatibility flag.

The new test case effectively verifies that when ATTR_ENUMERATED_COERCION is explicitly enabled, the draggable="false" attribute is preserved correctly and the appropriate deprecation warning is emitted. This complements the existing test and follows the established testing patterns in the file.

🧰 Tools

🪛 Biome (1.9.4)

[error] 288-288: Don't use 'Function' as a type.

Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.

(lint/complexity/noBannedTypes)

skirtles-code

A few more thoughts on the test...

packages/vue-compat/__tests__/misc.spec.ts

…eck attribute

yangxiuxiu1115 marked this pull request as draft November 20, 2024 10:27

yangxiuxiu1115 marked this pull request as ready for review November 20, 2024 11:42

edison1105 added 🔨 p3-minor-bug Priority 3: this fixes a bug, but is an edge case that only affects very specific usage. scope: v2 compat labels Nov 21, 2024

edison1105 added the ready to merge The PR is ready to be merged. label Nov 21, 2024

skirtles-code reviewed May 20, 2025

View reviewed changes

yangchangtao added 4 commits May 27, 2025 17:36

fix(compat): fix compat handler of draggable

5b59155

chore: update

d996c6a

chore: testcase

06fd879

chore: update

2a58947

yangxiuxiu1115 force-pushed the fix/compat-attr branch from f006bad to 2a58947 Compare May 27, 2025 09:36

yangchangtao added 2 commits May 27, 2025 17:41

chore: update

c67368a

chore: update

5e0e684

coderabbitai bot reviewed May 27, 2025

View reviewed changes

skirtles-code reviewed May 27, 2025

View reviewed changes

packages/vue-compat/__tests__/misc.spec.ts Outdated Show resolved Hide resolved

packages/vue-compat/__tests__/misc.spec.ts Outdated Show resolved Hide resolved

packages/vue-compat/__tests__/misc.spec.ts Outdated Show resolved Hide resolved

edison1105 added 2 commits November 24, 2025 10:27

Merge branch 'main' into fix/compat-attr

a56b4ab

test(compat): update ATTR_ENUMERATED_COERCION test to include spellch…

102f560

…eck attribute

edison1105 requested a review from skirtles-code November 24, 2025 07:27

edison1105 merged commit ed85953 into vuejs:main Dec 18, 2025
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(compat): fix compat handler of draggable#12445

fix(compat): fix compat handler of draggable#12445
edison1105 merged 8 commits intovuejs:mainfrom
yangxiuxiu1115:fix/compat-attr

yangxiuxiu1115 commented Nov 20, 2024 •

edited by coderabbitai bot

Loading

Uh oh!

github-actions bot commented Nov 20, 2024 •

edited

Loading

Uh oh!

pkg-pr-new bot commented Nov 20, 2024 •

edited

Loading

Uh oh!

edison1105 commented Nov 21, 2024

Uh oh!

vue-bot commented Nov 21, 2024

Uh oh!

skirtles-code May 20, 2025

Uh oh!

skirtles-code May 20, 2025

Uh oh!

coderabbitai bot commented May 27, 2025 •

edited

Loading

Uh oh!

coderabbitai bot left a comment

Uh oh!

skirtles-code left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

yangxiuxiu1115 commented Nov 20, 2024 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

github-actions bot commented Nov 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Size Report

Bundles

Usages

Uh oh!

pkg-pr-new bot commented Nov 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

edison1105 commented Nov 21, 2024

Uh oh!

vue-bot commented Nov 21, 2024

Uh oh!

skirtles-code May 20, 2025

Choose a reason for hiding this comment

Uh oh!

skirtles-code May 20, 2025

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot commented May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Suggested reviewers

Poem

Pre-merge checks and finishing touches

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

skirtles-code left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

yangxiuxiu1115 commented Nov 20, 2024 •

edited by coderabbitai bot

Loading

github-actions bot commented Nov 20, 2024 •

edited

Loading

pkg-pr-new bot commented Nov 20, 2024 •

edited

Loading

coderabbitai bot commented May 27, 2025 •

edited

Loading