fix(compat): fix compat handler of draggable #12445
Conversation
Size ReportBundles
Usages
|
@vue/compiler-core
@vue/compiler-dom
@vue/compiler-sfc
@vue/compiler-ssr
@vue/runtime-core
@vue/reactivity
@vue/runtime-dom
@vue/server-renderer
@vue/shared
vue
@vue/compat
commit: |
|
/ecosystem-ci run |
|
📝 Ran ecosystem CI: Open
|
| @@ -500,3 +500,11 @@ test('local app config should not affect other local apps in v3 mode', () => { | |||
| const app2 = createApp({}) | |||
| expect(app2.config.globalProperties.test).toBe(undefined) | |||
| }) | |||
|
|
|||
| test('ATTR_ENUMERATED_COERCION: true', () => { | |||
There was a problem hiding this comment.
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?
| value === null || value === false || value === 'false' | ||
| ? 'false' | ||
| : typeof value !== 'boolean' && value !== undefined | ||
| : value !== undefined |
There was a problem hiding this comment.
I think the logic is correct, but it might be easier to understand if the undefined check were handled first?
f006bad to
2a58947
Compare
WalkthroughThe internal logic for coercing enumerated attribute values in the Changes
Sequence Diagram(s)sequenceDiagram
participant TestRunner
participant VueInstance
participant compatCoerceAttr
participant DOM
TestRunner->>VueInstance: Mount with compatConfig ATTR_ENUMERATED_COERCION: true
VueInstance->>compatCoerceAttr: Coerce 'draggable' attribute with value 'false'
compatCoerceAttr-->>VueInstance: Return 'false'
VueInstance->>DOM: Render <div draggable="false">
VueInstance->>TestRunner: Emit deprecation warning
TestRunner->>TestRunner: Assert rendered HTML and warning
Assessment against linked issues
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
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
Functionas 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
📒 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_COERCIONis explicitly enabled, thedraggable="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)
|
|
||
| test('ATTR_ENUMERATED_COERCION: true', () => { | ||
| const vm = new Vue({ | ||
| compatConfig: { ATTR_ENUMERATED_COERCION: true }, |
There was a problem hiding this comment.
Is this needed? I think this is enabled in this file by default.
| @@ -275,3 +275,17 @@ test('ATTR_ENUMERATED_COERCION', () => { | |||
| )('contenteditable', 'foo', 'true'), | |||
| ).toHaveBeenWarned() | |||
| }) | |||
|
|
|||
| test('ATTR_ENUMERATED_COERCION: true', () => { | |||
There was a problem hiding this comment.
The test name should be more specific. Something like:
| test('ATTR_ENUMERATED_COERCION: true', () => { | |
| test('ATTR_ENUMERATED_COERCION, coercing "false"', () => { |
| test('ATTR_ENUMERATED_COERCION: true', () => { | ||
| const vm = new Vue({ | ||
| compatConfig: { ATTR_ENUMERATED_COERCION: true }, | ||
| template: `<div><div draggable="false">hello</div></div>`, |
There was a problem hiding this comment.
I think we should also test boolean false. e.g.:
| template: `<div><div draggable="false">hello</div></div>`, | |
| template: `<div><div draggable="false" :spellcheck="false">hello</div></div>`, |
Plus the relevant assertions.
fix #12444
Summary by CodeRabbit
Bug Fixes
Tests