[sitecore-jss-react] Memory leak in SitecoreContext

[sc-illiakovalenko]

Description

Investigation for performance issue of node-headless-proxy
Purpose: get rid of subscribers, use setState instead of forceUpdate.
I'll cover by unit-tests if my changes could be accepted

Motivation

I prepared flame graph, you can download it, where you can see that with fix - SitecoreContext is not hottest
Also you can look at results.txt, it's result of load tests, 150 req, 20 users.
clinic flame --on-port 'artillery quick --count 150 -n 20 http://localhost:3000' -- node index.js
Flame Graph: flames.zip

1. We should use setState instead of forceUpdate. React recommend it: https://reactjs.org/docs/react-component.html#forceupdate
2. We don't need subscribers, because it's not working like sockets, when app opened by 10 clients will share data at any time.
   At every request server push new subscriber, and that's it, server forget about new subscriber, because it can't do anything with it. subscribers - like a dead storage, that consumes more and more memory on every request, because we need to go through the all subscribers and call forceUpdate on every request of the page. We can't unsubscribe, because componentWillUnmount is working only on client side.
3. Single subscriber is enough, so on every request - server will replace subscriber, call forceUpdate one time and send it to the client. On the client side we have ONLY one subscriber.
4. super(props, context) is deprecated, instead have to use: super(props)

How Has This Been Tested?

• e2e tests
• manually

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation update (non-breaking change; modified files are limited to the /docs directory)

Checklist:

• I have read the Contributing guide.
• My code follows the code style of this project.
• My code/comments/docs fully adhere to the Code of Conduct.
• My change is a code change and it requires an update to the documentation.
• My change is a documentation change and it requires an update to the navigation.

[sc-illiakovalenko]

Now, removed this part from RouteHandler in constructor, because it's not recommended by react to force some updates from constructor.

      SitecoreContextFactory.setSitecoreContext({	
        route: ssrInitialState.sitecore.route,	
        itemId: ssrInitialState.sitecore.route.itemId,	
        ...ssrInitialState.sitecore.context,	
      });

So, before my fix, every component that was wrapped by withSitecoreContext was rendered two times. Now it's rendering only one time, because we set correct context before RouteHandler is rendered

[nickwesselman]

Would be good to get some additional eyes internally or from the community on this breaking change. Any feedback from @kamsar @aweber1 @GaryWenneker @erzr?

[kamsar]

The original implementation of this was my code (sorry!) and it was to deal with some oddities involving nested placeholders and React updates.

I don't 100% recall the exact details, but due to the way we dynamically compose components in the Placeholder component, we were having issues causing proper component invalidation when context data updated down multiple placeholders - the placeholder props weren't changing, so neither were the children even being evaluated for re-rendering so we opted to force update the specific components that subscribed to it.

[aweber1]

First, some history, then some feedback :)

IIRC, one of the reasons for having a setSitecoreContext function and the concept of pub/sub in the <SitecoreContext /> component was to make Layout Service context data available to components that might be rendered outside of the <SitecoreContext /> component tree. For instance, global navigation or some other "static" component(s) that don't rely on Layout Service route data but may need to update on route change based on the context data returned by Layout Service.

I don't believe the sample app demonstrates that type of usage, I think it was carried over from the original React "Advanced App".

I also think much of the context-related code was built around the early (early) React Context API, which was not as stable or as functional as it is now.

All of that said, I would probably recommend that the sample app be simplified and the responsibility of handling pub/sub of Layout Service context data be offloaded to documentation or implementation example.

A few suggestions to that end:

The typical caveats apply to all code below - it's all un-tested and is intended to be illustrative.

• In my opinion, <Router /> / <RouteHandler /> should be ancestors of <SitecoreContext />. In this hierarchy, the <SitecoreContext /> component will be updated more naturally when a route change occurs. Something like this:

<Router>
  <RouteHandler>
    <SitecoreContext>
      <Layout>

• Using React state within a React context provider seems a bit unnatural to me. When a context value is updated, any context consumers should automatically be notified via normal React context mechanics. State should be managed outside of the context in my opinion.

• For "static" components that may live outside of <Router /> that still need access to Layout Service context data, you could add a function to the <SitecoreContext /> component that is called when it receives new context data, e.g. onContextUpdated. An app could then add "listeners" to that function to update external/static components. Something like this:

// module App.js

function App ({ initialState }) {

return (
<div>
  <StaticComponent contextData={initialState.sitecore.context} />
  <Router>
    <RouteHandler ssrInitialState={initialState} />
  </Router>
</div>
);
}

// module `server.js`

// NOTE: don't call `RouteHandler.setServerSideRenderingState`, that function should be removed altogether.
// Instead, pass initial SSR `state` to `<AppRoot />`. 

renderToStringWithData(
  <AppRoot initialState={state} path={path} Router={StaticRouter} graphQLClient={graphQLClient} />
)

// module `RouteHandler.js`

// Refactor this component to receive `ssrInitialState` as a prop instead of "injected" by 
// `setServerSideRenderingState`. Then the component becomes something more like the below.
// This is obviously over-simplified code that doesn't account for language changes and edge
// cases, it is more for illustration.

import { emitContextEvent } from './contextEventSystem';

function RouteHandler({ ssrInitialState, route }) {
  const [state, setState] = useState({ data: ssrInitialState, route, loading: false });
  
  useEffect(() => {
    if (route !== state.route) {
      setState({ loading: true, data: state.data, route: state.route });
      fetchRouteData().then((routeData) => {
        setState({ loading: false, data: routeData, route });
      })
    }
  });
  
  if (state.loading) {
    return <LoadingComponent />;
  }
  
  if (state.data) {
    return (
      <SitecoreContext contextData={state.sitecore.context} onContextUpdated={emitContextEvent}>
        <Layout route={state.sitecore.route} />
      </SitecoreContext>
    );
  }
}

// module `StaticComponent.js`

import { useState } from 'react';
import { subscribeToContextEvent } from './contextEventSystem';

export function StaticComponent ({ contextData }) {
  const [state, setState] = useState(contextData);
  
  subscribeToContextEvent('static-component', (newContextData) => {
    setState(newContextData);
  });
  
  return <div>Static Component using `state` value is rendered here.</div>;
}

// module `contextEventSystem.js`

const eventListeners = {};

export function subscribeToContextEvent(subscriberId, eventHandler) {
  eventListeners[subscriberId] = eventHandler;
}

export function emitContextEvent(sitecoreContextData) {
  for (const subscriberId in eventListeners) {
    eventListeners[subscriberId](sitecoreContextData);
  }
}

[nickwesselman]

If we are talking about exposing data/state outside the Router, isn't that a bigger problem that should/could be solved by access to the entirety of the Layout Service response? IIRC, our original examples actually got their navigation data from context (via the getLayoutServiceContext pipeline). But I think it's a more common practice to put that navigation data into a component w/ a custom Contents Resolver, right?

So what are the use cases for the StaticComponent? Are they common? If a developer really wants Layout Service data outside the Router, should we leave it to the their implementation to do something custom with it in their RouteHandler? Like implement a delegate subscription as you describe @aweber1 , vs us building it in to SitecoreContext?

[nickwesselman]

Worth noting too that React docs show examples of updating context from a nested component. But here they do keep the state on the App vs in the provider.

https://reactjs.org/docs/context.html#updating-context-from-a-nested-component

[sc-illiakovalenko]

Worth noting too that React docs show examples of updating context from a nested component. But here they do keep the state on the App vs in the provider.

https://reactjs.org/docs/context.html#updating-context-from-a-nested-component

@aweber1 Yes, it's recommended to store as state. Why can't we wrap whole application at the root level by SitecoreContext? I thought it's a normal practice. So all components can access to context using withSitecoreContext? Is it not a solution?
Usually in React, Context defined at the root level, so we don't need to split our components on some types. RouteHandler will be a child of SitecoreContext and all components outiside of RouteHandler - too

[aweber1]

TBH, I think removing the use of forceUpdate and context factory is the primary reason for enhanced SSR performance and fixed memory leak.

Beyond that, I think we're in more of a philosophical discussion about React state management and JSS sample app architecture - a separate discussion if necessary but not particularly germane to the issue attached to this PR.

[nickwesselman]

@aweber1 True, but that philosophical discussion does affect what goes into the restructured SitecoreContext in sitecore-jss-react... in any case you've given us something to think about. Thanks for jumping in.