sysgears / apollo-universal-starter-kit

Apollo Universal Starter Kit is a SEO-friendly, fully-configured, modular starter application that helps developers to streamline web, server, and mobile development with cutting-edge technologies and ultimate code reuse.
https://apollokit.org
MIT License
1.68k stars 323 forks source link

Define a pattern for dependency injection for client modules #629

Open adam-s opened 6 years ago

adam-s commented 6 years ago

I'm working on some custom client modules. One has a side navigation for an administration dashboard using Ant Design Layout Sider. I can either hard code the menu into the module or I can hack the connector.web.jsx file to include navItemSide.

Perhaps, instead of hard coding specific configuration like menu, menu right, menu side navigation which doesn't exist, dependency injection can be used instead. I've looked at a couple third party repositories that might be useful including react dependency injection (react.di) and InversifyJS. The idea is the index.js or index.web.js for each module will have a constructor class which defines which modules are injected into it.

This approach will make it much easier later on for third party contributed modules such as the collapsible side navigation module I'm working on right now. There needs to be a way to validate dependent modules are installed. The modules can be added and removed at will.

I had been working with Drupal for years and this is a great way for a large community to work with a modular project.

adam-s commented 6 years ago

I was thinking about this this morning. When the app is initially bootstrapping all the modules get an opportunity to define imperative configuration. jQuery plugins are imperative meaning information about how to render an item is sent in a configuration object $.myPlugin({ imperativeWrapper: '<div class="wrapper"><%=varToBeWrapped%></div>'}) which is something that React is trying to get away from. This is why most big React apps have flat directories, all the components are in one big folder and all the containers are in another. So what happens with jQuery plugins is that the configuration object keeps growing every time a new developer needs to add a feature like translation. This is the most anti-pattern in React.

I need a side nav and now I have to add another property to the feature configuration object.

Perhaps, instead of all the dependency inject stuff (which, by the way, if done correctly would be awesome as developers can create and add very sophisticated third party modules) the connector.web.jsx holds a single config object without the several getter methods. There is a config object and a single getter method that takes a key.

import modules from '../../../../../../modules';
{modules.navItems}

becomes...

import modules from '../../../../../../modules';
{modules.getConfig('navItems')}

The connector.web.jsx file should not know how to reduce all the individual configuration objects which it does now. If the layout wants to render nav items, it should just ask for the configuration object and reduce it how it sees fit. There might be a place for some helper function to manage configuration objects and trees.

adam-s commented 6 years ago

I went ahead and started changing how configuration is stored on the client. It's just a cache with a getter and setter.

https://github.com/adam-s/apollo-universal-starter-kit/blob/configuration/packages/client/src/modules/configConnector.web.jsx

https://github.com/adam-s/apollo-universal-starter-kit/commit/63912ba2bf3ae7d5b2972147a5d486aefcaea2db

I ran into a problem with the new configuration file can't be imported with webpack under certain circumstances which might mean there is a circular dependency. However, it might have more to do with trying to import react components before React is fully bootstrapped. Meaning, things work if setTimeout() is used to move everything to the back of the event loop task queue.

It is frustrating because managing the config this way makes 100% more sense because new modules can simply add different configuration and the modules which consume configuration are responsible for defining how to use and reduce it.

Maybe there should app level configuration like reducers and resolvers on one hand and React level configuration which is managed inside a configuration provider so that it isn't called or executed before React is bootstrapped.

Just some ideas I've been thinking about. I'm quite disappointed I couldn't get it to work.

Most likely the place where this is going wrong is in packages/client/src/modules/common/components/web. The NavBar component isn't really stateless as it imports the configuration which although doesn't call Navbar does have a fewimport * statements down the dependency tree that creates the circular dependency which webpack doesn't resolve. Perhaps, the NavBar component shouldn't be asking for state but rather have it passed to it from a top level menus module in a prop.

larixer commented 6 years ago

@adam-s Sorry, this all is difficult to comprehend. What do you want to do? Could you be more specific and focus on describing what do you want to do, without sending off to official docs on various sites. Could you provide minimalistic code samples that describe your problem concisely? Without crystal clear understanding of your root problem, it is difficult to propose anything.

adam-s commented 6 years ago

Sorry about that. When I get free time, I will clarify more of my thoughts on how to build an extendable configuration which doesn't hard code features like nav items. Developers should be able to add and remove modules without pieces still in the core code of the app. I've been thinking about several ways other projects accomplish this.

larixer commented 6 years ago

@adam-s I'm asking not about that. I'm asking about your initial problem that you tried to solve, not about solutions that you come up with.

adam-s commented 6 years ago

I added a third navigation menu and wanted each module to be able to add nav menu items to it without changing files which will be overwritten when pulling the latest version of the app from GitHub.

Morphexe commented 6 years ago

Since I was thinking about this too: correct me if I am wrong @adam-s but what you want is, just like the connector has getNavItems() and getNavItemsRigh() you want to be able to add something like getDashboardRoutes right? the problem is that you don't want to hack the connector to add this method, so you are attempting to rely on DI to add this sort of stuff to the connector so that you can then get them from any of your modules as needed.

This could be a good addition , I was thinking in a naive approach this morning (like getRoutes(Id) and when adding routes to the connector just add them with an id and then resolving based on that Id, this is error-prone, but would work on my case) but DI might be the best way to go.

Depending on the DI framework, some imports might be simplyfied, since for example in the navbar instead of importing settings, we could have DI Settings Module and import it from the DIContext, instead of directly importing modules and settings.

I might take a shot at this once I have some free time.

larixer commented 6 years ago

@adam-s @Morphexe If you implement YOUR Dashboard you should write YOUR connector to link YOUR components into this Dashboard, you don't need to hack kits connector for that. You can have many different connectors in your app, that connect YOUR components to other YOUR components. The connectors supplied by the kit are here to connect modules to the kit core. If you will need another way to connect modules to the core of the kit, sure the connectors provided by the kit must be modified, but this is not surprising.

Morphexe commented 6 years ago

@vlasenko I was talking generally about the connector not dashboard/routes specific, but more in a way to improve the current connector architecture to allow easy extensions without having to create multiple connectors for different uses. If I achieve a working architecture with this requirements, I might submit a pull request for you to look at and decided if its worth it or not.

larixer commented 6 years ago

@Morphexe I bet what you are really looking for is improving modularity of the kit core. No matter what you will use, connectors, hooks, dependency injection, something else. It doesn't really matter. The core features of the kit must be extremely modular so that you can hook into core feature of the kit in all practical cases. And to make them cover all practical cases we all should contribute back and make them more modular then they are now. The contributions to make core of the kit more modular will certainly be accepted and highly appreciated. But throwing off connectors and replacing them with {whatever} is not going to work.

adam-s commented 6 years ago

@vlasenko I don't think my idea is to throw off connectors. I believe that connectors should be defined by modules using them either by getting a piece of configuration or setting a piece of configuration. If the API makes it easy for developers to share their individual modules, the system will be very powerful.

Some things that are common for an API like this is defining the modules name and then having other modules list dependencies. The API would check to make sure all the dependencies are installed throwing a warning if one is missing. This way one developer can contribute a dashboard module which uses the sidenav module another developer made.

I don't think it is that complicated because what is happening is the connector is becoming dumb. It doesn't know about navItems anymore. It just has an API for modules to say they are looking for navItems and others to add navItems.

It is just something to think about. Nonetheless, I strongly believe if you have a modular system that isn't opinionated making it easy for other developers to contribute modules to a community there will be a much stronger adaption of the whole project. I was brain storming some of the many ways other successful projects accomplished this. I find the lack of this a major pain point for developing react.

larixer commented 6 years ago

@adam-s It is the idea of the starter kit to eventually make it easy to contribute modules like that. And we move in this direction.

That's why we have utilized monorepo in master - to split frontend, backend, mobile frontend into three separate packages and use Yarn Workspaces, otherwise such splitting would be unbearable at some point because of the time to install 3 copies of shared node_modules for each package. The next step will be to split each module into its own package so that features were able to depend on each other via standard npm mechanisms. And we need to improve spinjs to quickly create such module-features with right dependencies, part of the logic to do that is already there, but there is much more work to be done there.

That's why we process all the sources the same way via Webpack, for server, mobile platforms, and web platform. This is needed for the system like you describe, but that was not the case for JavaScript at all yet. There is tiny amount of projects that process backend sources via the same build tools as frontend code. There is tiny amount of projects where React Native sources are built with Webpack - only Haul does that, but they don't support Expo and hence require setting up native mobile development tooling with Gradle, etc, hence they go outside of JavaScript and make the system you want unachievable. We also use React Native Web to make UI components written once and run everywhere possible. This is also needed for the system that you describe.

So yeah, we are heading right in this direction, but there are lots of stuff to be done to arrive there.

Morphexe commented 6 years ago

This is exactly the info I was looking forward. I never intended to throw off conectors, in fact I wanted to extend the core functionality in a more modular fashion, to allow easy imports/exports I have a plan to implement this, lets see how it goes.

verdverm commented 6 years ago

I've been working on a Connector library which would enable this capability.

https://github.com/hofstadter-io/connector-js

Should have a branch up soon demonstrating against this repository.

Morphexe commented 6 years ago

@verdverm yeah! I saw that on the chat, and found it a awesome idea! ;)

verdverm commented 6 years ago

POC here: https://github.com/sysgears/apollo-universal-starter-kit/pull/645

verdverm commented 6 years ago

@adam-s you can get the 'Get' functionality today if you make class Feature extends Connector from https://github.com/hofstadter-io/connector-js

(You will probably want to make all the module Features -> Connector, keeping only the root Feature in client & server) ((switch the imports in the modules))

If you want to use DI:

var clientFeatures = Feature(...features);
clientFeature.Connect(clientFeature); // self-recursive ;]

All of your modules will now get the root connector/feature, which has all of the connectors / features

if they define a function Connect: (connector) => {...}

in which you can do anything you want, including

var items = connector.Get({rightDashMenuItem: true})

(Note, the value is ignored, only keys from the Get arg are looked at)