Sometimes `resolvedData is undefined` and that causes 22k issues per-day

[sidoruk-sv]

Related Sentry issue: https://sentry.wixpress.com/sentry/business-manager/issues/40846805

Related code: https://github.com/wix/react-module-container/blob/master/src/react-loadable-component.js#L13

Don't know the right solution for now, and is that was expected behaviour? For me that sounds more like a bug in this package.

[ronenst]

This can happen through BM-flow if one of the parallel requests fails, since it will pass undefined in that case. https://github.com/wix-private/yoshi/blob/c712bd348240c75c162b7332f3865e59273bc380/packages/yoshi-flow-bm-runtime/src/createLazyComponent.tsx#L101

I can simulate this error for example by blocking one of the translation files:

[ronenst]

To clarify, I did the translation since it's the easiest, but I believe it's likely one of the API requests is failing

[kobiburnley]

The issue is with resourceLoader promise.

It's being initialized in base-lazy-component: https://github.com/wix/react-module-container/blob/3400a99de0af7e8635aef2a7fd8c3342e2f530b4/src/base-lazy-component.js#L23

And errors are caught here: https://github.com/wix/react-module-container/blob/3400a99de0af7e8635aef2a7fd8c3342e2f530b4/src/base-lazy-component.js#L27

At this phase resolvedData is undefined and resourceLoader promise is fullfiled - because of the catch handler.

And then ReactLoadableComponent continues the promise chain, and crashes when trying to read resolvedData: https://github.com/wix/react-module-container/blob/3400a99de0af7e8635aef2a7fd8c3342e2f530b4/src/react-loadable-component.js#L13

[kobiburnley]

Actually this bug interrupts bm-flow from bubbling up the real error up the react tree:

https://github.com/wix-private/yoshi/blob/c712bd348240c75c162b7332f3865e59273bc380/packages/yoshi-flow-bm-runtime/src/createLazyComponent.tsx#L116

[ronenst]

But resolvedData is coming from the resolve callback which is in the bm-flow code, so it's possible for bm-flow to catch this itself, or am I missing something?

I agree it's a problem the module-registry swallows the error but we can't break this behaviour now safely, this is why we added the event emitter. Previously we passed this on to sentry in BM but it was pretty noisy so we shut it down.

[kobiburnley]

resolvedData is never set because the resolve function rejects.

[kobiburnley]

In other words, if the following code was written in TS, we'd get a type check error, because resolvedData is nullable:

        const component = this.resolvedData.default || this.resolvedData;        
       this.setState({component});

I think that checking for nulls here would fix the issue, and also bm-flow would have bubble up the original error up the react tree.

[ronenst]

resolvedData is never set because the resolve function rejects.

Yes, but resolve is coming from you, right? So you can catch the error instead of rejecting?

But I think a null check makes sense too.

[ranyitz]

There are currently 60% of business-manager modules on bm-flow, and 20% of them which are on yoshi@5.

If we'll be able to solve this issue here, the fix will be applied to 100% of the modules, instantly (after deploying the business-manager).

If we can do both ways, I prefer the second one.

[kobiburnley]

I wonder if now because this bug is fixed, when an error occurs in resolve, users will keep seeing the spinner, instead of the "Something went wrong" placeholder (BM flow bubbles up the error, I refer to non BM flow projects)

[ronenst]

@ranyitz I don't know how much this applies to non-flow projects though, obviously there is an issue (several, actually...) with this module but I believe fixing it should be considered a breaking change unless we can be sure it isn't we can also do it with an FT and/or some sort of flag that bm-flow sets in the manifest.