Svelte 5 implementation

[nicklozon]

Resolves #130

I've translated the React and Vue components to Svelte 5.

Please forgive me if I've made any major mistakes or made poor decisions - I attempted to keep the implementation as similar to the Vue implementation as possible.

Here are the major notes that identify differences, nuances, and decisions made.

• The svelte package was initialized using the Svelte CLI and choosing the "component library" option. It does use SvelteKit to bundle by copying *.svelte files in src/lib. There may be a better way to bundle, but I was unsuccessful after hours of playing with basic Svelte and Vite.
  • SvelteKit does not build a UMD module
  • SvelteKit doesn't actually "bundle" any JS at all - just seems to copy the js files. I'm not very familiar with building and bundling, but I've published a package and it worked fine in the demo app. https://www.npmjs.com/package/inertiaui-modal-svelte/v/0.0.1?activeTab=code
• Tests and demo-app were expanded to handle the third stack option, sometimes requiring functions and case statements instead of inline if statements
• Melt UI headless component library was used for the default Modal - handles escape key-press and clicking outside to close the modal
  • Does not set aria-hidden on app mount point, so this was added manually so tests pass. Melt UI adheres to WAI-ARIA design patterns, but setting aria-hidden seems to be something other headless libraries do
  • Configuration options are exposed through the HeadlessModal in Vue, but this creates a race condition with Melt UI as it needs to know closeExplicitly before the headless modal is even mounted. This is duplication that is suggests configuration could be more centrally managed.
• Vue and React renderApp seem to both be very different. Vue uses some hyperscript function to render the App inside the ModalRoot - hyperscript seems to allow rendering components programmatically outside of a component. I don't know of a way to do this with Svelte, so I simply rendered the App inside ModalRoot by passing App as a prop.
• Vue performs prop filtering in ModalRenderer when passing props to the page component - there doesn't seem to be a way to infer what props a component uses in Svelte, so all props are passed to the page.
• The page component loading in ModalRenderer had to be appended with .default - seems like imports aren't being treated as ECMAScript modules from the component resolver or Svelte doesn't treat components as named exports. I believe this is outside the scope of the project unless there's some javascript configuration somewhere that changes the behaviour.
• I was using Svelte 5.35 and having issues with $derived.by for onTopOfStack - updating to latest fixed the issue, so I'm recommending ^5.36.0 as the svelte version to use
• Svelte $state does not support reassignment, meaning stack = [] would not trigger for components expecting reactive update. For this reason the $state variables exported in modalStack.svelte.js should never be reassigned. I.e. stack.length = 0 resets the stack entirely.

I'll be happy to update the documentation if this is to get adopted.

[williamthome]

Friendly ping @pascalbaljet :)

[williamthome]

Claude Code Review

# Code Review: Svelte 5 Implementation (PR #140)

PR: #140
Author: nicklozon
Status: OPEN
Changes: +8751 additions, -25 deletions

Overview

This PR adds comprehensive Svelte 5 support to the inertiaui/modal library, bringing it to parity with the existing React and Vue implementations. The implementation includes:

• Complete Svelte 5 components and utilities
• Demo app integration with all test scenarios
• CI/CD pipeline updates for automated testing
• Melt UI integration for accessible headless modals

Analysis

Strengths

1. Comprehensive Implementation

• All core features from React/Vue implementations have been ported
• Test coverage appears complete with demo-app integration
• CI/CD properly configured to test across all stacks

2. Modern Svelte 5 Approach

• Uses runes ($state, $derived, $props) appropriately
• Proper reactivity patterns with $state constraints documented
• Context API used correctly for modal state management

3. Accessibility

• Melt UI integration ensures WAI-ARIA compliance
• Manual aria-hidden additions where needed
• Keyboard navigation and focus management included

4. Documentation of Decisions

• PR description thoroughly documents implementation choices
• Key differences from React/Vue implementations noted
• Known limitations and workarounds explained

Issues & Suggestions

1. Critical: State Mutation Pattern (svelte/src/lib/modalStack.svelte.js)

// Current approach mentioned in PR:
stack.length = 0  // Resets stack

This is a footgun. While it works, it's non-idiomatic and could confuse developers. Consider:

• Using clear() method instead: stack.clear()
• Or documenting this pattern prominently in code comments
• Better: wrap in a proper API method like clearStack()

2. Build System Concerns
The PR mentions SvelteKit doesn't bundle JS, just copies files. This could lead to:

• Larger bundle sizes for consumers
• Duplicate dependencies
• Tree-shaking issues

Recommendation: Investigate Vite library mode or rollup directly for proper bundling.

3. Component Import Pattern (ModalRenderer.svelte)

// Requires .default appending
component.default

This suggests the module resolution isn't correctly configured. Check:

• vite.config.js resolve settings
• Ensure component exports are consistent
• This workaround may break with different bundlers

4. Missing Prop Filtering
Unlike Vue which filters props in ModalRenderer, Svelte passes all props to page components. While not breaking, this could:

• Expose internal implementation details
• Lead to prop naming conflicts
• Make debugging harder

Suggestion: Implement prop filtering similar to Vue, or document this as a known limitation.

5. Race Condition with Melt UI
The PR mentions closeExplicitly configuration causes race conditions:

needs to know `closeExplicitly` before the headless modal is even mounted

This suggests architectural issues. Consider:

• Making configuration static/synchronous
• Using Svelte 5's $effect.pre() for pre-mount setup
• Centralizing config management as suggested in PR

6. CI/CD Matrix Complexity
Adding Svelte increases test matrix significantly (4 stacks × 2 Inertia versions × 2 mounting modes). Consider:

• Parallel test execution optimization
• Selective testing for non-breaking changes
• Cost/time implications documented

Code Quality

Positive:

• Consistent naming conventions
• Good use of Svelte 5 features
• Error handling appears solid
• TypeScript types included (seen in package.json dependencies)

Concerns:

• svelte/src/lib/modalStack.svelte.js needs internal documentation
• Some files are very large (EditUser.svelte is 144 lines)
• Consider splitting complex components into smaller pieces

Test Coverage

From the diff, tests cover:

• Auto vs custom mounting
• Modal lifecycle (open/close/reload)
• Event communication between modals
• Props passing
• Form submissions
• Nested modals

Missing Tests:

• Error boundary scenarios
• Memory leak checks (important with reactive state)
• Accessibility automated tests (beyond aria-hidden)

Security Considerations

No obvious security issues, but:

• Ensure XSS protection in ModalRenderer when rendering dynamic components
• Validate props passed to page components
• Review Melt UI security posture

Performance Implications

Concerns:

1. Bundle Size: No UMD build means potentially larger bundles
2. Reactivity Overhead: Multiple $derived computations could impact performance with many modals
3. No Lazy Loading: All components appear to be loaded upfront

Recommendations:

• Add bundle size analysis to CI
• Consider lazy loading for modal components
• Profile with realistic modal stacks (5+ modals)

Breaking Changes

None for existing React/Vue users. New Svelte implementation is additive.

Documentation Needs

1. svelte/README.md: Should document:

   • State mutation patterns (stack.length = 0)
   • .default requirement for imports
   • Melt UI dependency and customization
   • Svelte version requirement (^5.36.0)

2. Migration Guide: For users coming from other stacks

3. Examples: The demo-app serves as examples, but standalone docs would help

Recommendations

High Priority

1. Fix Build System: Investigate proper bundling instead of file copying
2. Document State Patterns: Add clear documentation about $state mutation constraints
3. Resolve Import Pattern: Fix .default requirement or document why it's necessary
4. Add Prop Filtering: Implement similar to Vue or document the difference

Medium Priority

5. Centralize Config: Address the closeExplicitly race condition architecturally
6. Performance Testing: Add bundle size checks and performance benchmarks
7. Accessibility Tests: Add automated a11y testing
8. Component Splitting: Break down large components (>100 lines)

Low Priority

9. CI Optimization: Consider parallel execution optimization
10. TypeScript Strictness: Review TypeScript configuration for strictness
11. Examples: Add standalone examples beyond demo-app

Verdict

Recommendation: Request Changes

This is a solid implementation that demonstrates clear understanding of Svelte 5 and the existing codebase. However, the build system concerns and architectural issues (state mutation pattern, config race conditions, import patterns) should be addressed before merging.

Specific Requests:

1. Investigate and fix the build/bundling approach
2. Document or fix the .default import requirement
3. Add prominent documentation about state mutation patterns
4. Consider implementing prop filtering for consistency

After these changes, this PR should be ready to merge. The author has done excellent work documenting decisions and maintaining parity with existing implementations.

---

Reviewed by: Claude Code
Date: 2025-10-30

[nicklozon]

Thanks William - since this has some attention, I'll rebase main and freshen it up since it's stale.

Edit: Rebased and changes made. inertiaui-modal-svelte v0.0.4 published.