mirror of
https://github.com/The-Low-Code-Foundation/OpenNoodl.git
synced 2026-01-12 15:22:55 +01:00
236 lines
6.9 KiB
Markdown
236 lines
6.9 KiB
Markdown
# TASK-008: EventDispatcher + React Hooks Investigation
|
|
|
|
## Status: 🟡 Investigation Needed
|
|
|
|
**Created**: 2025-12-22
|
|
**Priority**: Medium
|
|
**Complexity**: High
|
|
|
|
---
|
|
|
|
## Overview
|
|
|
|
During Task 004B (ComponentsPanel React Migration), we discovered that the legacy EventDispatcher pub/sub pattern does not work with React hooks. Events are emitted by legacy models but never received by React components subscribed in `useEffect`. This investigation task aims to understand the root cause and propose long-term solutions.
|
|
|
|
---
|
|
|
|
## Problem Statement
|
|
|
|
### What's Broken
|
|
|
|
When a React component subscribes to ProjectModel events using the EventDispatcher pattern:
|
|
|
|
```typescript
|
|
// In useComponentsPanel.ts
|
|
useEffect(() => {
|
|
const handleUpdate = (eventName: string) => {
|
|
console.log('🔔 Event received:', eventName);
|
|
setUpdateCounter((c) => c + 1);
|
|
};
|
|
|
|
const listener = { handleUpdate };
|
|
|
|
ProjectModel.instance.on('componentAdded', () => handleUpdate('componentAdded'), listener);
|
|
ProjectModel.instance.on('componentRemoved', () => handleUpdate('componentRemoved'), listener);
|
|
ProjectModel.instance.on('componentRenamed', () => handleUpdate('componentRenamed'), listener);
|
|
|
|
return () => {
|
|
ProjectModel.instance.off('componentAdded', listener);
|
|
ProjectModel.instance.off('componentRemoved', listener);
|
|
ProjectModel.instance.off('componentRenamed', listener);
|
|
};
|
|
}, []);
|
|
```
|
|
|
|
**Expected behavior**: When `ProjectModel.renameComponent()` is called, it emits 'componentRenamed' event, and the React hook receives it.
|
|
|
|
**Actual behavior**:
|
|
|
|
- ProjectModel.renameComponent() DOES emit the event (verified with logs)
|
|
- The subscription code runs without errors
|
|
- BUT: The event handler is NEVER called
|
|
- No console logs, no state updates, complete silence
|
|
|
|
### Current Workaround
|
|
|
|
Manual refresh callback pattern (see NOTES.md for details):
|
|
|
|
1. Hook provides a `forceRefresh()` function that increments a counter
|
|
2. Action handlers accept an `onSuccess` callback parameter
|
|
3. Component passes `forceRefresh` as the callback
|
|
4. Successful actions call `onSuccess()` to trigger manual refresh
|
|
|
|
**Problem with workaround**:
|
|
|
|
- Creates tech debt
|
|
- Must remember to call `onSuccess()` in ALL code paths
|
|
- Doesn't scale to complex event chains
|
|
- Loses the benefits of reactive event-driven architecture
|
|
|
|
---
|
|
|
|
## Investigation Goals
|
|
|
|
### Primary Questions
|
|
|
|
1. **Why doesn't EventDispatcher work with React hooks?**
|
|
|
|
- Is it a closure issue?
|
|
- Is it a timing issue?
|
|
- Is it the context object pattern?
|
|
- Is it React's StrictMode double-invocation?
|
|
|
|
2. **What is the scope of the problem?**
|
|
|
|
- Does it affect ALL React components?
|
|
- Does it work in class components?
|
|
- Does it work in legacy jQuery Views?
|
|
- Are there any React components successfully using EventDispatcher?
|
|
|
|
3. **Is EventDispatcher fundamentally incompatible with React?**
|
|
- Or can it be fixed?
|
|
- What would need to change?
|
|
|
|
### Secondary Questions
|
|
|
|
4. **What are the migration implications?**
|
|
|
|
- How many places use EventDispatcher?
|
|
- How many are already React components?
|
|
- How hard would migration be?
|
|
|
|
5. **What is the best long-term solution?**
|
|
- Fix EventDispatcher?
|
|
- Replace with modern state management?
|
|
- Create a React bridge?
|
|
|
|
---
|
|
|
|
## Hypotheses
|
|
|
|
### Hypothesis 1: Context Object Reference Mismatch
|
|
|
|
EventDispatcher uses a context object for listener cleanup:
|
|
|
|
```typescript
|
|
model.on('event', handler, contextObject);
|
|
// Later:
|
|
model.off('event', contextObject); // Must be same object reference
|
|
```
|
|
|
|
React's useEffect cleanup may run in a different closure, causing the context object reference to not match, preventing proper cleanup and potentially blocking event delivery.
|
|
|
|
**How to test**: Try without context object, or use a stable ref.
|
|
|
|
### Hypothesis 2: Stale Closure
|
|
|
|
The handler function captures variables from the initial render. When the event fires later, those captured variables are stale, causing issues.
|
|
|
|
**How to test**: Use `useRef` to store the handler, update ref on every render.
|
|
|
|
### Hypothesis 3: Event Emission Timing
|
|
|
|
Events might be emitted before React components are ready to receive them, or during React's render phase when state updates are not allowed.
|
|
|
|
**How to test**: Add extensive timing logs, check React's render phase detection.
|
|
|
|
### Hypothesis 4: EventDispatcher Implementation Bug
|
|
|
|
The EventDispatcher itself may have issues with how it stores/invokes listeners, especially when mixed with React's lifecycle.
|
|
|
|
**How to test**: Deep dive into EventDispatcher.ts, add comprehensive logging.
|
|
|
|
---
|
|
|
|
## Test Plan
|
|
|
|
### Phase 1: Reproduce Minimal Case
|
|
|
|
Create the simplest possible reproduction:
|
|
|
|
1. Minimal EventDispatcher instance
|
|
2. Minimal React component with useEffect
|
|
3. Single event emission
|
|
4. Comprehensive logging at every step
|
|
|
|
### Phase 2: Comparative Testing
|
|
|
|
Test in different scenarios:
|
|
|
|
- React functional component with useEffect
|
|
- React class component with componentDidMount
|
|
- Legacy jQuery View
|
|
- React StrictMode on/off
|
|
- Development vs production build
|
|
|
|
### Phase 3: EventDispatcher Deep Dive
|
|
|
|
Examine EventDispatcher implementation:
|
|
|
|
- How are listeners stored?
|
|
- How are events emitted?
|
|
- How does context object matching work?
|
|
- Any special handling needed?
|
|
|
|
### Phase 4: Solution Prototyping
|
|
|
|
Test potential fixes:
|
|
|
|
- EventDispatcher modifications
|
|
- React bridge wrapper
|
|
- Migration to alternative patterns
|
|
|
|
---
|
|
|
|
## Success Criteria
|
|
|
|
This investigation is complete when we have:
|
|
|
|
1. ✅ Clear understanding of WHY events don't reach React hooks
|
|
2. ✅ Documented root cause with evidence
|
|
3. ✅ Evaluation of all potential solutions
|
|
4. ✅ Recommendation for long-term fix
|
|
5. ✅ Proof-of-concept implementation (if feasible)
|
|
6. ✅ Migration plan (if solution requires changes)
|
|
|
|
---
|
|
|
|
## Affected Areas
|
|
|
|
### Current Known Issues
|
|
|
|
- ✅ **ComponentsPanel**: Uses workaround (Task 004B)
|
|
|
|
### Potential Future Issues
|
|
|
|
Any React component that needs to:
|
|
|
|
- Subscribe to ProjectModel events
|
|
- Subscribe to NodeGraphModel events
|
|
- Subscribe to any EventDispatcher-based model
|
|
- React to data changes from legacy systems
|
|
|
|
### Estimated Impact
|
|
|
|
- **High**: If we continue migrating jQuery Views to React
|
|
- **Medium**: If we keep jQuery Views and only use React for new features
|
|
- **Low**: If we migrate away from EventDispatcher entirely
|
|
|
|
---
|
|
|
|
## Related Documentation
|
|
|
|
- [LEARNINGS.md](../../../reference/LEARNINGS.md#2025-12-22---eventdispatcher-events-dont-reach-react-hooks)
|
|
- [Task 004B Phase 5](../TASK-004B-componentsPanel-react-migration/phases/PHASE-5-INLINE-RENAME.md)
|
|
- EventDispatcher implementation: `packages/noodl-editor/src/editor/src/shared/utils/EventDispatcher.ts`
|
|
- Example workaround: `packages/noodl-editor/src/editor/src/views/panels/ComponentsPanelNew/hooks/useComponentActions.ts`
|
|
|
|
---
|
|
|
|
## Timeline
|
|
|
|
**Status**: Not started
|
|
**Estimated effort**: 1-2 days investigation + 2-4 days implementation (depending on solution)
|
|
**Blocking**: No other tasks currently blocked
|
|
**Priority**: Should be completed before migrating more Views to React
|