Skip to content
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

Will onModelChange cause all the components rendered inside the factory to be re-rendered after modifying the model? #456

Open
daigang1228 opened this issue Sep 25, 2024 · 7 comments

Comments

@daigang1228
Copy link

daigang1228 commented Sep 25, 2024

Describe the bug

Will onModelChange cause all the components rendered inside the factory to be re-rendered after modifying the model?

Your Example Website or App

Steps to Reproduce the Bug or Issue

Modify the layout so that you can see the content under each tab flashing

Expected behavior

Modifying the layout should not re-render the content of each tab

Operating System

mac

Browser Type?

chrome

Browser Version

128.0.6613.120

Screenshots or Videos

Additional context

No response

@nealus
Copy link
Collaborator

nealus commented Sep 25, 2024

The model should be set, then mutated using Actions, rather than recreated for each change in the layout.

The optional onModelChange() callback is to allow processing after the model has changed (due to an action).

@daigang1228
Copy link
Author

daigang1228 commented Sep 25, 2024

@nealus
But there will be no issue in v0.7.3 version
The re-rendering problem will only occur in 0.8.1

@ericsvendsen
Copy link

Can confirm. Was running v0.7.15 and everything was fine. Upgraded to v0.8.1 and my app is completely hosed with "maximum update depth exceeded" error messages. Checked the changelog for breaking changes for v0.8, and nothing at all is mentioned that points to this issue. I'm fine to stay on 0.7.15 for now, but any further guidance here would be much appreciated. Thank you!

@rob-myers
Copy link

@nealus in [email protected] you can recreate the Model with different JSON to e.g.

  • remove tab(s)
  • add tabs(s)
  • provide tab(s) with different props

all in-one-go, without remounting any existing tabs.

This is a very helpful feature missing from [email protected].

@bvanseg
Copy link

bvanseg commented Dec 4, 2024

This is blocking us from upgrading to 0.8.0+. It would be very helpful if there was an action to update just the layout JSON of the model. Without the ability to do so, layout synchronization via websocket across multiple devices becomes a challenge after the changes made in 0.8.0+, as the layout updating on one end triggers the entire model on another end to be recreated.

Alternatively, maybe decoupling the layout from the model altogether would be best? Letting the model become a dispatcher for actions, while the layout itself can be updated freely as a separate prop or by some other means.

@paralin
Copy link
Contributor

paralin commented Feb 3, 2025

I am running into the same issue. My app deterministically generates the Model after changes are applied and passes a new Model (different by reference) to the Layout. The previous versions (0.7.x) would not re-render all the tabs in this case. The new version re-renders all the tabs (unmounting the DOM and re-mounting), which is of course terrible for performance.

Reproduction of the issue: https://github.com/paralin/flex-layout-issue-3454

When you click "change layout" it passes an identical Model with Model.fromJson(sameJsonModel) and causes a re-render (new component instance created).

@paralin
Copy link
Contributor

paralin commented Feb 5, 2025

Here was what I came up with:

import React, { PureComponent } from 'react'
import {
  Layout,
  Model,
  TabNode,
  ILayoutProps,
  Action,
  Rect,
} from '@aptre/flex-layout'

/**
 * Optimized Layout Demo
 * 
 * This demonstrates an efficient tab rendering approach for flex-layout:
 *
 * 1. Tab Container Strategy:
 *    - Maintains a container div before the Layout component
 *    - Holds all tab content components
 *    - Uses absolute positioning for visible tabs
 *    - Keeps hidden tabs in DOM with display: none
 *
 * 2. Tab Reference System:
 *    - TabRef components manage visibility/positioning
 *    - Updates CSS properties without re-rendering content
 *    - Handles mount/unmount/resize events
 *
 * 3. Performance Benefits:
 *    - Avoids re-rendering tab contents
 *    - Only updates CSS properties for layout changes
 *    - Maintains component state when hiding/showing tabs
 */

interface ITabRefProps {
  tab: TabNode
  layout: OptimizedLayout 
}

// TabRef manages visibility and positioning of a tab
class TabRef extends PureComponent<ITabRefProps> {
  private resizeListener = (params: { rect: Rect }) => {
    this.updatePosition(params.rect)
  }

  private visibilityListener = (params: { visible: boolean }) => {
    const { tab, layout } = this.props
    if (params.visible) {
      layout.showTab(tab.getId())
    } else {
      layout.hideTab(tab.getId())
    }
  }

  componentDidMount() {
    const { tab, layout } = this.props
    layout.showTab(tab.getId())
    this.updatePosition(tab.getRect())
    tab.setEventListener('resize', this.resizeListener)
    tab.setEventListener('visibility', this.visibilityListener)
  }

  componentWillUnmount() {
    const { tab, layout } = this.props
    layout.hideTab(tab.getId())
    tab.removeEventListener('resize')
    tab.removeEventListener('visibility')
  }

  updatePosition(rect: Rect) {
    if (!rect) return
    this.props.layout.positionTab(this.props.tab.getId(), rect)
  }

  render() {
    return null
  }
}

interface ITabProps {
  tabID: string
  visible?: boolean
  rect?: Rect
  children: React.ReactNode
}

// Tab renders the actual tab content
const Tab: React.FC<ITabProps> = ({ tabID, visible, rect, children }) => {
  return (
    <div
      role="tabpanel"
      aria-labelledby={`tab-button-${tabID}`}
      data-tab-id={tabID}
      style={{
        position: 'absolute',
        display: visible ? 'flex' : 'none',
        left: rect?.x,
        top: rect?.y,
        width: rect?.width,
        height: rect?.height,
      }}
    >
      {children}
    </div>
  )
}

interface IOptimizedLayoutProps {
  model: Model
  renderTab: (tabId: string) => React.ReactNode
  layoutProps?: Partial<ILayoutProps>
}

// OptimizedLayout demonstrates the tab optimization approach
export class OptimizedLayout extends PureComponent<IOptimizedLayoutProps> {
  // Track visible tabs and their positions
  private tabRefs: {
    [tabID: string]: {
      visible: boolean
      rect?: Rect
    }
  } = {}

  showTab = (tabID: string) => {
    this.tabRefs[tabID] = {
      ...this.tabRefs[tabID],
      visible: true,
    }
    this.forceUpdate()
  }

  hideTab = (tabID: string) => {
    this.tabRefs[tabID] = {
      ...this.tabRefs[tabID],
      visible: false,
    }
    this.forceUpdate()
  }

  positionTab = (tabID: string, rect: Rect) => {
    this.tabRefs[tabID] = {
      ...this.tabRefs[tabID],
      rect,
    }
    this.forceUpdate()
  }

  private createComponent = (node: TabNode) => {
    return <TabRef tab={node} layout={this} />
  }

  private onModelChange = (model: Model, action: Action) => {
    // Handle model changes if needed
    console.log('Model changed:', action)
  }

  render() {
    const { model, renderTab, layoutProps } = this.props

    return (
      <>
        <Layout
          model={model}
          factory={this.createComponent}
          onModelChange={this.onModelChange}
          {...layoutProps}
        />
        <>
          {Object.entries(this.tabRefs).map(([tabID, ref]) => (
            <Tab
              key={tabID}
              tabID={tabID}
              visible={ref.visible}
              rect={ref.rect}
            >
              {renderTab(tabID)}
            </Tab>
          ))}
        </>
      </>
    )
  }
}

It renders tabs in a separate <div> and uses position: absolute to position them in the correct space when visible. When invisible we use display: none. This optimizes away the problem of Model causing a re-render of the Layout.

I will have a look at adding this to flex-layout as well.

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

No branches or pull requests

6 participants