VulcanJS / Vulcan

🌋 A toolkit to quickly build apps with React, GraphQL & Meteor
http://vulcanjs.org
MIT License
7.98k stars 1.89k forks source link

Rethink how Components object is accessed? #2153

Open SachaG opened 5 years ago

SachaG commented 5 years ago

Currently the global Components object that stores all Vulcan components (so you can use them with e.g. <Components.MovieList/>) is accessed with an import.

Other patterns we could consider:

@eric-burel recently added a withComponents HoC that implements option 1, and we are now using it for a few key components. The question is whether that approach should be extended to all components… Let's discuss.

Personally I think that the HoC approach is interesting (it offers more flexibility), and we could even make it transparent by wrapping every single component with the HoC transparently whenever you call registerComponent. A big downside though is the React DOM pollution that would result… So at this stage I don't think it's worth it.

I'm curious to know if React Hooks could somehow give us the flexibility that the import lacks but without the downsides of the HoC though?

eric-burel commented 5 years ago

When do we need Components?

Before thinking of importing Components, here my point of view on the usage of the Components registry. I'd use Components/registerComponents:

How we get the Components?

Using a global variable that acts as a component registry.

So far so good. Pros: easiest pattern, does not overcomplicate things, makes sense. Cons: override is tedious, you have to manually merge the imported global with your local components. You use the registry directly, so there is no room for more features, things are tied.

Using the new withComponents HOC.

Pros: provides additional feature, like allowing to locally replace one or more components using the components prop. Global import is only in one place, so we may avoid multiple sources of truth. Makes things more explicit, and thus more testable (you can hardly replace a global when writing tests while you can easily switch/remove an HOC)

Cons: tedious pattern: you have to either pass down the Components props to all child of the wrapped components. For the moment it's usage is limited to the DataTable and could be extended to the Form. The HOC is exposed, so people might use it for their own reusable components.

I think having both patterns is already a nice thing, that's enough to write reusable-and-customizable components. Yet there might be room for improvements.

How could we improve this?

By wrapping everybody when registering the component?

The problem is that not every component is registered. And registered components does not necessarily mean it uses the global Components object. So I am not fond of this, that seems to much magic for me with not enough escape hatches.

Using Context?

Context would solve the issue with passing down the Components to child. Exactly like we do for the Apollo Client, the Redux Store, the Material UI theme, i18n and so on. But using the Context API is tricky and can lead to bug. If someone with a previous experience with Context passes by...

Using Hooks?

Hooks do not apply to classes, and they are meant for lifecycle methods. But I am not close-minded about this: this is a new pattern, pretty sure it will lead to awesome innovations, maybe it could indeed be applied here. Anyway hooks will helps us a lot into refactoring core components (I look at you, SmartForm).

RenderProps?

renderProps are meant to create abstract components: you write your UI logic, and delegate render to submethods. You can pass aditionnal params to those methods. Passing a components props to core components to customize them is actually quite similar, only advantage of renderProps is that you can avoid name clashes by renaming props. We could solve this already by naming our prop vulcanComponents for example.


All this is quite experimental, I'd be really glad to get feedback on this :)

Apollinaire commented 5 years ago

I think my preferred option is Eric's implemented solution with the HOC. The problem is more prop drilling, but I think it's fine. The benefit from this feature is huge, it really gives full customizability without bloating the framework too much.

SachaG commented 5 years ago

I agree. I think all we need to do is use context somehow to avoid the prop drilling issue and it'll be great.