-
-
Notifications
You must be signed in to change notification settings - Fork 379
feat: compatible Vue3 vapor mode for v11 #2320
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThis PR adds support for Vue 3.6's GenericComponentInstance type across the vue-i18n core library. Changes include broadening component instance type unions in internal maps and function signatures, shifting devtools linkage from DOM-based to instance-based attachment, extracting getCurrentInstance into a local utils module, and updating the Vue version override from 3.5.13 to 3.6.0-alpha.4. Changes
Sequence Diagram(s)sequenceDiagram
participant Component as Vue Component<br/>(ComponentInternalInstance or<br/>GenericComponentInstance)
participant I18n as I18n Core
participant Devtools as Devtools Integration
participant Utils as Utils
Note over Component,Utils: Old Flow (Vue 3.5)
Component->>I18n: Mount with ComponentInternalInstance
I18n->>I18n: Store in __instances map<br/>(keyed by ComponentInternalInstance)
I18n->>Devtools: Link via vnode.el.__VUE_I18N__
Devtools->>Devtools: Check componentInstance.vnode.el.__VUE_I18N__
Note over Component,Utils: New Flow (Vue 3.6+)
Component->>I18n: Mount with ComponentInternalInstance<br/>or GenericComponentInstance
I18n->>I18n: Store in __instances map<br/>(keyed by ComponentInternalInstance |<br/>GenericComponentInstance)
I18n->>Component: Assign instance.__VUE_I18N__<br/>(direct instance attachment)
Devtools->>Component: Check componentInstance.__VUE_I18N__<br/>(direct instance lookup)
Devtools-->>Devtools: Simplified resolution logic
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
|
@coderabbitai |
|
@kazupon I'll review this PR for you right away! Let me analyze the changes for Vue 3 Vapor mode compatibility. ✅ Actions performedReview triggered.
|
@intlify/core
@intlify/core-base
@intlify/devtools-types
@intlify/message-compiler
petite-vue-i18n
@intlify/shared
vue-i18n
@intlify/vue-i18n-core
commit: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (1)
package.json (1)
158-160: Consider aligningvuedevDependency with the pnpm override
devDependencies.vueis still3.5.13whilepnpm.overrides.vueforces3.6.0-alpha.4. That works, but it’s easy to misread the effective Vue version from the manifest. Consider bumping the devDependency to the same version or adding a short comment in release notes/PR description so future readers understand why an alpha override is in place.Also applies to: 187-189
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
package.json(1 hunks)packages/vue-i18n-core/src/composer.ts(4 hunks)packages/vue-i18n-core/src/devtools.ts(5 hunks)packages/vue-i18n-core/src/i18n.ts(11 hunks)packages/vue-i18n-core/src/utils.ts(3 hunks)packages/vue-i18n-core/test/i18n.test.ts(1 hunks)packages/vue-i18n-core/test/issues.test.ts(1 hunks)packages/vue-i18n-core/test/wc.test.ts(1 hunks)
⏰ 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). (4)
- GitHub Check: release
- GitHub Check: Build (macos-latest, 20)
- GitHub Check: Build (ubuntu-latest, 20)
- GitHub Check: Build (windows-latest, 20)
🔇 Additional comments (5)
packages/vue-i18n-core/test/i18n.test.ts (1)
37-37: Tests now use the sharedgetCurrentInstancehelperSwitching the test import to
../src/utilskeeps tests aligned with the runtime abstraction and avoids depending directly on Vue’sgetCurrentInstanceAPI.packages/vue-i18n-core/test/issues.test.ts (1)
33-33: Issue tests updated to use utilsgetCurrentInstancePointing these tests at
../src/utilsis consistent with the new runtime abstraction and avoids hard-coding Vue internals in the test suite.packages/vue-i18n-core/test/wc.test.ts (1)
5-19: WC tests aligned with runtime helpers and fallback registrationRegistering the compiler, resolver, and locale fallbacker and switching to
createI18n/I18nInjectionKey/getCurrentInstancemirrors the production setup for web components and custom blocks. This keeps the WC coverage representative of actual usage.Also applies to: 23-27
packages/vue-i18n-core/src/i18n.ts (1)
31-35: Instance tracking and lifecycle now handle bothComponentInternalInstanceandGenericComponentInstancecoherentlyUsing
ComponentInternalInstance | GenericComponentInstanceas the key type for__instances, widening the internal APIs (__getInstance/__setInstance/__deleteInstance,getI18nInstance,getComposer,getParentComponentInstance,setupLifeCycle), and attaching the composer totarget.__VUE_I18N__on mount (and deleting it on unmount) all align with the new utils/devtools contracts and Vue 3.6’s generic instances. TheisCEgate ingetI18nInstanceis a nice touch to route web components throughI18nInjectionKeywhile regular components still use the app’s plugin symbol.Please run the updated WC tests and a parent-scope
useI18n({ useScope: 'parent' })scenario under your targeted Vue 3.6/Vapor build to confirm that:
- custom elements correctly resolve the injected i18n via
I18nInjectionKey, and- parent-scope resolution via
getParentComponentInstancebehaves as expected withGenericComponentInstance.Also applies to: 49-53, 79-106, 262-296, 523-548, 837-854, 875-913, 915-926, 928-966
packages/vue-i18n-core/src/composer.ts (1)
41-42: Clarify the TypeScript breaking change for release notesThe web search confirms the assignability issue is more definitive than stated: functions explicitly typed with
instance: ComponentInternalInstanceare not assignable to the widened callback understrictFunctionTypes: true(not just "may see stricter assignability"). This is a breaking change that should be clearly documented in release notes with explicit mention of thestrictFunctionTypesrequirement and recommended migration path (e.g., switching to callback-inferred typing or expanding handler types toComponentInternalInstance | GenericComponentInstance).
| import type { | ||
| App, | ||
| ComponentInternalInstance, | ||
| GenericComponentInstance | ||
| } from 'vue' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
Devtools now bind to instance.__VUE_I18N__, consistent with new lifecycle wiring
Reading the composer from componentInstance.__VUE_I18N__ (and tagging tree nodes only when it differs from the global composer) matches how setupLifeCycle in i18n.ts now attaches/removes the composer on component instances, including generic ones. The widened roots and getComponentInstance typings to ComponentInternalInstance | GenericComponentInstance are in line with that model.
Also applies to: 72-97, 115-118, 168-189, 321-327
🏁 Script executed:
#!/bin/bash
# Inspect all uses of `__VUE_I18N__` to ensure devtools and lifecycle wiring are consistent.
rg -n "__VUE_I18N__" packages/vue-i18n-core -C3Length of output: 7001
Critical inconsistency: devtools reads from component instance, but mixin still attaches to DOM element
The verification reveals a mismatch between binding locations:
i18n.ts(setupLifeCycle, composition API): attaches__VUE_I18N__to the component instance (target.__VUE_I18N__)devtools.ts: reads from the component instance (componentInstance.__VUE_I18N__)mixins.ts(options API): attaches__VUE_I18N__to the DOM element (this.$el.__VUE_I18N__)
The devtools code will not discover composers attached via the mixin path, breaking devtools inspection for legacy/options-API components. Either mixins.ts needs to attach to the component instance instead of the DOM element, or devtools needs to fall back to checking the DOM element for legacy compatibility.
🤖 Prompt for AI Agents
In packages/vue-i18n-core/src/devtools.ts around lines 27 to 31, devtools
currently only reads __VUE_I18N__ from the component instance but mixins attach
it to the DOM element; update devtools to fallback to checking the component's
root DOM element if componentInstance.__VUE_I18N__ is undefined (i.e., check
componentInstance.vnode?.el or componentInstance?.proxy?.$el for __VUE_I18N__),
so it discovers composers attached via the options/mixin path; implement the
fallback lookup and use whichever composer object is found.
| import * as Vue from 'vue' | ||
| import { Text, createVNode } from 'vue' | ||
| import { I18nWarnCodes, getWarnMessage } from './warnings' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
Centralized getCurrentInstance/GenericComponentInstance support looks consistent
Importing * as Vue and introducing getCurrentInstance() plus a ComponentInternalInstance | GenericComponentInstance-aware getComponentOptions() lines up with the wider instance usage in composer, i18n, and devtools. The Vue.currentInstance || Vue.getCurrentInstance() fallback is a reasonable way to bridge Vue 3.6 behavior with earlier runtimes in one place.
Also applies to: 19-23, 168-172, 222-228
🏁 Script executed:
#!/bin/bash
# Check for any remaining direct `getCurrentInstance` imports/usages now that utils exposes a wrapper.
rg -n "getCurrentInstance" packages/vue-i18n-core -C3Length of output: 9739
Update mixins.ts to use the getCurrentInstance wrapper from utils
The centralization goal is undermined: packages/vue-i18n-core/src/mixins.ts imports getCurrentInstance directly from 'vue' (line 2) instead of from './utils'. This bypasses the Vue 3.6 compatibility wrapper entirely. Lines 44 and 134 in mixins.ts should use the wrapper to ensure consistent handling.
Change line 2 in mixins.ts:
- import { getCurrentInstance } from 'vue'
+ import { getCurrentInstance } from './utils'
🤖 Prompt for AI Agents
In packages/vue-i18n-core/src/mixins.ts around lines 44 and 134, the file
imports getCurrentInstance directly from 'vue' instead of using the
compatibility wrapper in ./utils; update the import statement to import
getCurrentInstance from './utils' (remove it from the named imports from 'vue'
and add it to the local utils import) so both usages at the specified lines call
the wrapper, preserving Vue 3.6 compatibility and centralized behavior.
back-ported from master branch (v12)
related #2299
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.