refactor: remove Vue.js dependency from island custom element#63
refactor: remove Vue.js dependency from island custom element#63
Conversation
Replace Vue's defineCustomElement with a plain HTMLElement subclass so the entry script no longer bundles Vue. Vue now loads on-demand via dynamic import alongside each island component at hydration time.
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe custom element implementation transitions from Vue's Changes
Sequence Diagram(s)sequenceDiagram
participant DOM as Browser/DOM
participant VueIsland as VueIsland<br/>Web Component
participant Vue as Vue Library
participant EntryModule as Entry Module
participant VueApp as Vue SSR App
DOM->>VueIsland: connectedCallback()
VueIsland->>VueIsland: Read entry & serialized-props<br/>attributes
VueIsland->>Vue: Dynamic import
VueIsland->>EntryModule: Dynamic import
VueIsland->>Vue: createSSRApp(component, props)
Vue->>VueApp: Create app instance
VueIsland->>VueApp: mount(element)
VueApp->>VueIsland: Mounted & rendered
DOM->>VueIsland: disconnectedCallback()
VueIsland->>VueApp: unmount()
VueApp->>VueIsland: Cleanup complete
VueIsland->>VueIsland: Clear `#app` reference
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/client/custom-element.ts (1)
23-23: Consider wrappingJSON.parsein try-catch for robustness.If
serialized-propscontains malformed JSON, this will throw an uncaught exception. While the server should produce valid JSON, adding defensive error handling would improve resilience.🛡️ Proposed fix
- const parsedProps = JSON.parse(serializedProps) + let parsedProps: Record<string, unknown> + try { + parsedProps = JSON.parse(serializedProps) + } catch { + console.error('[vue-island] Failed to parse serialized-props:', serializedProps) + return + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/custom-element.ts` at line 23, Wrap the JSON.parse call that produces parsedProps from serializedProps in a try-catch to avoid uncaught exceptions on malformed input: catch any SyntaxError (or generic Error), log or handle the failure appropriately (e.g., fallback to an empty object or return early) and ensure parsedProps is set to a safe default; update the logic where parsedProps is used so downstream code can handle the fallback case. Target the JSON.parse(serializedProps) expression and the variable parsedProps when making this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/custom-element.ts`:
- Around line 9-27: connectedCallback is async and can race with
disconnection/reconnection; before and after awaiting imports you must guard
against stale runs and avoid mounting to a disconnected element. Add a
per-instance run token/version (increment on each connectedCallback entry) and
store it (e.g., this._connectToken); if an existing this.#app exists when
starting a new connection, unmount and clear it; after the Promise.all resolve,
check that the current token still matches and that this.isConnected is true
before calling createSSRApp(...).mount(this); if the check fails, do not mount
(and clean up any created app) so stale async work cannot mount into a removed
element.
---
Nitpick comments:
In `@src/client/custom-element.ts`:
- Line 23: Wrap the JSON.parse call that produces parsedProps from
serializedProps in a try-catch to avoid uncaught exceptions on malformed input:
catch any SyntaxError (or generic Error), log or handle the failure
appropriately (e.g., fallback to an empty object or return early) and ensure
parsedProps is set to a safe default; update the logic where parsedProps is used
so downstream code can handle the fallback case. Target the
JSON.parse(serializedProps) expression and the variable parsedProps when making
this change.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
test/__snapshots__/hydration.test.ts.snapis excluded by!**/*.snaptest/__snapshots__/prod.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (1)
src/client/custom-element.ts
| async connectedCallback(): Promise<void> { | ||
| const entry = this.getAttribute('entry') | ||
| if (!entry) { | ||
| return | ||
| } | ||
|
|
||
| const serializedProps = this.getAttribute('serialized-props') ?? '{}' | ||
|
|
||
| const [{ createSSRApp }, module] = await Promise.all([ | ||
| import('vue'), | ||
| import(/* @vite-ignore */ entry), | ||
| ]) | ||
|
|
||
| const entryComponent = module.default | ||
| const parsedProps = JSON.parse(serializedProps) | ||
|
|
||
| this.#app = createSSRApp(entryComponent, parsedProps) | ||
| this.#app.mount(this) | ||
| } |
There was a problem hiding this comment.
Race condition: element may be disconnected before mount completes.
Since connectedCallback is async, the element could be removed from DOM while awaiting the imports. Mounting to a disconnected element can cause hydration issues. Additionally, if the element is rapidly reconnected, a second connectedCallback can start before the first completes, orphaning the first app instance.
🛡️ Proposed fix to guard against race conditions
async connectedCallback(): Promise<void> {
const entry = this.getAttribute('entry')
if (!entry) {
return
}
const serializedProps = this.getAttribute('serialized-props') ?? '{}'
const [{ createSSRApp }, module] = await Promise.all([
import('vue'),
import(/* `@vite-ignore` */ entry),
])
+ // Guard against disconnection during async import or concurrent connectedCallback calls
+ if (!this.isConnected || this.#app) {
+ return
+ }
+
const entryComponent = module.default
const parsedProps = JSON.parse(serializedProps)
this.#app = createSSRApp(entryComponent, parsedProps)
this.#app.mount(this)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async connectedCallback(): Promise<void> { | |
| const entry = this.getAttribute('entry') | |
| if (!entry) { | |
| return | |
| } | |
| const serializedProps = this.getAttribute('serialized-props') ?? '{}' | |
| const [{ createSSRApp }, module] = await Promise.all([ | |
| import('vue'), | |
| import(/* @vite-ignore */ entry), | |
| ]) | |
| const entryComponent = module.default | |
| const parsedProps = JSON.parse(serializedProps) | |
| this.#app = createSSRApp(entryComponent, parsedProps) | |
| this.#app.mount(this) | |
| } | |
| async connectedCallback(): Promise<void> { | |
| const entry = this.getAttribute('entry') | |
| if (!entry) { | |
| return | |
| } | |
| const serializedProps = this.getAttribute('serialized-props') ?? '{}' | |
| const [{ createSSRApp }, module] = await Promise.all([ | |
| import('vue'), | |
| import(/* `@vite-ignore` */ entry), | |
| ]) | |
| // Guard against disconnection during async import or concurrent connectedCallback calls | |
| if (!this.isConnected || this.#app) { | |
| return | |
| } | |
| const entryComponent = module.default | |
| const parsedProps = JSON.parse(serializedProps) | |
| this.#app = createSSRApp(entryComponent, parsedProps) | |
| this.#app.mount(this) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/custom-element.ts` around lines 9 - 27, connectedCallback is async
and can race with disconnection/reconnection; before and after awaiting imports
you must guard against stale runs and avoid mounting to a disconnected element.
Add a per-instance run token/version (increment on each connectedCallback entry)
and store it (e.g., this._connectToken); if an existing this.#app exists when
starting a new connection, unmount and clear it; after the Promise.all resolve,
check that the current token still matches and that this.isConnected is true
before calling createSSRApp(...).mount(this); if the check fails, do not mount
(and clean up any created app) so stale async work cannot mount into a removed
element.
1 participant
Summary
defineCustomElementwith a plainHTMLElementsubclass insrc/client/custom-element.tsimport('vue')alongside each island component at hydration time, keeping the entry script Vue-free<slot>projectionTest plan
pnpm buildpassespnpm lintpassespnpm test --run)Summary by CodeRabbit