Skip to content

feat(test): add vitest setup and widget container tests #3102

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

Merged
merged 1 commit into from
Jul 28, 2025

Conversation

tearf001
Copy link
Contributor

Description

This PR fixes the issue where multiple GridStack instances would crash when using the React integration due to shared state in the static GridStack.renderCB callback (#2937).

Problem: The static GridStack.renderCB was being shared across all GridStack instances, causing widget container references to be mixed up between different grids. This led to crashes and incorrect behavior when multiple grids were present on the same page.

Solution: Implemented a WeakMap-based approach to store widget containers separately for each GridStack instance:

• Added a gridWidgetContainersMap WeakMap that uses the GridStack instance as the key
• Each grid instance now maintains its own isolated widget container map
• The WeakMap ensures automatic memory cleanup when grid instances are destroyed

the fix snippets:

// Before: All grids shared the same widget containers
GridStack.renderCB = (el, widget) => {
  widgetContainersRef.current.set(widget.id, el); // Shared state
}

// After: Each grid has its own container map
GridStack.renderCB = (el, widget) => {
  if (widget.grid) {
    let containers = gridWidgetContainersMap.get(widget.grid);
    if (!containers) {
      containers = new Map();
      gridWidgetContainersMap.set(widget.grid, containers);
    }
    containers.set(widget.id, el); // Instance-specific state
  }
}

This ensures that multiple GridStack instances can coexist safely without interfering with each other's widget management.

Checklist

  • All tests passing (yarn test)

Additional notes:

• Added comprehensive unit tests using Vitest to verify the WeakMap functionality
• Tests cover: multi-instance isolation, proper cleanup, and widget container management
• The solution maintains backward compatibility while fixing the multi-instance issue

fix (fix gridstack#2937) - fix multiple grids crash.  Implement WeakMap-based widget container tracking in GridStackRenderProvider

- Configure vitest with jsdom environment in vite.config.ts
- Add test scripts and dependencies to package.json

- Add comprehensive test suite for widget container management
@@ -2,121 +2,290 @@
# yarn lockfile v1


"@asamuzakjp/css-color@^3.2.0":
version "3.2.0"
resolved "https://registry.npmmirror.com/@asamuzakjp/css-color/-/css-color-3.2.0.tgz#cc42f5b85c593f79f1fa4f25d2b9b321e61d1794"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

guess you're using npm instead of yarn, which I guess is fine...

Copy link
Member

@adumesny adumesny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you for the changes. but those will get blown away when the p[ending React changelist comes in, so wondering if you could look at https://github.com//pull/2938 as well ?

@adumesny adumesny merged commit 143f417 into gridstack:master Jul 28, 2025
@tearf001 tearf001 deleted the fix-issue-2937-weakmap branch July 28, 2025 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants