search

wix-incubator / react-module-container

Small library for building micro apps in React and Angular
MIT License
69 stars 18 forks source link

add `isComponentRegistered` method #80

Closed just-jeb closed 4 years ago

just-jeb commented 4 years ago

Currently the only way to check if the component is registered is to invoke ModuleRegistry.component and check for result. However, in this case if the component is not registered there is an error thrown.
We need a noiseless way to check whether component registered or not, in order to invoke ModuleRegistry.component for registered components only to avoid unnecessary errors.

ronenst commented 4 years ago

Can you explain the use case? This shouldn't really be necessary if APIs are registered correctly.

Adding this method may encourage complexity in the consuming code.

just-jeb commented 4 years ago

@ronenst Sure. In the Dashboard we have widgets that are scoped to a specific auditory in the Dealer.
The Dashboard fetches the widget configurations from the dealer and then renders the relevant components, while getting them from ModuleRegistry.
Essentially what's rendered at the end is the intersection between widgets that match the scope of the user/site and widgets that are actually provided by BM modules.
There are two legitimate scenarios for not rendering a widget:

  1. Lazy component is provided by BM module but widget configuration doesn't return from Dealer (out of scope)
  2. Widget configuration returns from Dealer (matches the scope), but module doesn't provide the lazy component (experiments, vertical not installed whatever).

This PR should help to properly address scenario #2. Of course we could work it around by accessing ModuleRegistry['registeredComponents'] but we'd rather do it properly.

ronenst commented 4 years ago

Why not have a way for modules to register these widgets to your module independently from ModuleRegistry, that is, you should only be looking for the component if the peer module told that it exists?

The way you describe it, it sounds like you have a hard coded list of component but what you really want it to be is dynamic?

just-jeb commented 4 years ago

We actually do have a separate mechanism for registering widgets. Widgets registry holds the map between widgetId and BM component name, so it's independent from ModuleRegistry. But that's a fair point, if a widget hasn't been registered then there is no point to try and fetch it from ModuleRegistry. And we actually do fetch only components that are registered as widgets. So it's probably unnecessary. Closing, thanks for the swift response!

ronenst commented 4 years ago

Okay, you closed this but I started typing, so if you're interested I can try to expand on why I think this could be a problematic pattern.

I will point out though, that it's always possible to just call ModuleRegistry.component('nonExistant') and get undefined: https://github.com/wix/react-module-container/blob/master/src/module-registry.js#L49 (We're turning off the Sentry error spam for those cases).

While we already have apps that probably behave like this (as we see in Sentry) I don't think it should be defined behaviour.

Thinking forward about migrations to Module Federation, we can turn module communication to use regular imports instead of ModuleRegistry, so for example:

const SomeComponent = ModuleRegistry.component('someApp.SomeComponent');

Can become in the future:

import { SomeComponent } from 'someApp';

Using this syntax one will always expect that something happens, there is no analogy for the type of method that the PR adds and it would be a question how to migrate any logic between systems that relies on it.

just-jeb commented 4 years ago

Thanks for the explanation, it's a perfectly valid point. Although, we don't want to turn off Sentry because there might be a real issue, we'd like it to be clear from such errors naturally.
But as you said, this error shouldn't be popping if the widget is registered. Most of the UnregisteredComponent errors in our case are coming from tests (that actually check this behavior), so it should probably be taken with the relevant stakeholders.

ronenst commented 4 years ago

Well, I think if you try to mount it and it's undefined Sentry will see it and it will probably be a more relevant error and clearer stack trace.

ronenst commented 4 years ago

(Hoping to have a resilient solution for that type of case where a host won't completely collapse when that happens, just that widget that will be wrapped in an error boundary)