Extract duplicate switch statement into component

[erkde]

This commit just extracts the duplicate switch statement in index.jsx and ssr.jsx into it’s own thing, an App component.

As the default handling for an unknown prop componentName is different in index.jsx (throws an error) than in ssr.jsx (writes to console), the App component just returns null to indicate this condition.

App also takes an optional second prop data, in index.jsx it comes from window._react_data, I can’t find a type for it anywhere, so have left it as any for now.

[codecov-io]

Codecov Report

Merging #6658 into master will increase coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #6658      +/-   ##
==========================================
+ Coverage   95.75%   95.79%   +0.03%     
==========================================
  Files         294      294              
  Lines       26384    26321      -63     
  Branches     2018     2015       -3     
==========================================
- Hits        25265    25215      -50     
+ Misses        875      863      -12     
+ Partials      244      243       -1

Impacted Files	Coverage Δ
kuma/users/checks.py	41.17% <0%> (-0.93%)	⬇️
kuma/payments/views.py	95.23% <0%> (-0.32%)	⬇️
kuma/users/tasks.py	100% <0%> (ø)	⬆️
kuma/payments/tests/test_views.py	100% <0%> (ø)	⬆️
kuma/users/tests/test_stripe_subscription.py	100% <0%> (ø)	⬆️
kuma/settings/common.py	84.51% <0%> (ø)	⬆️
kuma/api/v1/tests/test_views.py	100% <0%> (ø)	⬆️
kuma/users/stripe_utils.py
kuma/users/utils.py	31.03% <0%> (ø)
kuma/users/views.py	94.19% <0%> (+1%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1692886...acba1e0. Read the comment docs.

[peterbe]

@Gregoor Can you take a look at this. I know you mentioned something similar and perhaps you can better visualize what's going on with on-going open pull requests that would conflict with this.

[Gregoor]

Thanks for starting to tackle this. I don't think we're quite there yet, but it's close!

[Gregoor]

React.createElement for which JSX is syntactical sugar always returns an object, describing the component to be rendered (and its props). What will be null is the renderer output. E.g.:

So the app === null checks in both components would always be false, hence this is a functional changes that prevents errors from being logged / thrown.

One workaround would be to let the component itself throw and then add an error boundary for the SSR component. It'd also be great if we had tests that made sure it's correctly catching and logging the error in that case then.

[erkde]

@Gregoor thanks for spotting this, and the comments.

I've just added a second commit c878df1 so it throws, along with a test to check ssr.jsx writes the caught error to console.

With adding an error boundary to the SSR component, I notice in index.jsx - the throw happens outside of the AppErrorBoundary, as it did before with the switch statement, I guess this is intentional, or should it actually be inside?

[erkde]

Hmm, I think my comment above is partially incorrect, it seems in index.jsx, the error is not thrown when the App component is assigned, e.g.

let app = <App componentName={componentName} data={data} />;

or when it is re-assigned to within the AppErrorBoundary, e.g.

let app = (
  <GAProvider>
    <AppErrorBoundary>
      <React.StrictMode>{app}</React.StrictMode>
    </AppErrorBoundary>
  </GAProvider>
);

but when app is passed to render, e.g.

ReactDOM.render(app, container);

So, I guess this PR's moves the error from being thrown outside of the AppErrorBoundary, to inside.

I'm not sure I understand why you would want to throw the error outside of it, but this does seem to change that.

[erkde]

One more thing, using error boundaries in ssr.jsx, doesn't seem to be currently supported with renderToString: facebook/react#10442

[erkde]

Note - has additional commit(s) based on review, may need a squash/edit.

[Gregoor]

Sorry for taking so long to get back to you and thanks for making the changes and adding tests!

I would've had one more nit, which is the fact that we're checking for the error message, instead of introducing a new error type. Then I learned that babel doesn't properly subclass and became sad. But now we have tests, so at least a mindless change to the error message blows CI up. So I guess we're good on that front :)

Regarding your notes:
Fortunately we don't rely on error boundaries in SSR atm, so as long as renderToString throws (which it does), we're good. And with index.jsx (aka the client bootstrap) the error throwing happens within the boundary (as we noticed before on-render, not on-create-element), so we can still control what users see as well as report the error to our analytics.

Thanks again for the patience with me and with Kuma (I just needed some time to figure out how to re-run the SSR piece myself, so double kudos for you).

[Gregoor]

Oh an if you want to look at another consolidation task (though more Python-heavy), here's one: #6678

[erkde]

Cool, thanks @Gregoor - happy to take a squiz at #6678