UIE-21 Code Structure and Dependencies PoC #3834
Conversation
- exploration of a custom DI Container pattern to allow for curated set of composable dependencies. - Goal is a scalable pattern to enable itterative and sustainable cross-team code sharing and cross-invemstment of high-value and non-trivial UI functionality like modals or "pages". - see src/dependencies for dependency core, definitions and defaults. - see src/appDependencies for app-level dependency lock-down - see src/pages/workspaces/analysis/modals/ExportAnalysisModal for component and component state usage examples.
| @@ -23,7 +23,7 @@ import { getUser } from 'src/libs/state' | |||
| import * as Utils from 'src/libs/utils' | |||
|
|
|||
|
|
|||
| window.ajaxOverrideUtils = { | |||
| (window as any).ajaxOverrideUtils = { | |||
There was a problem hiding this comment.
minimal conversion of ajax.js to .ts is not central to the DI patterns. I did this just to avoid type anoyances/polution of the core code bits elsewhere.
|
|
||
| export type ComposableAnalysisProvider = Composable<AnalysisProviderDeps, AnalysisProviderContract> | ||
|
|
||
| export const AnalysisProvider: ComposableAnalysisProvider = { |
There was a problem hiding this comment.
this provider is implemented in a pending PR for IA team, shown here to have an more robust example of the proposed UI stack.
| @@ -247,6 +247,8 @@ const Analyses = _.flow( | |||
| storageDetails: { googleBucketLocation, azureContainerRegion }, | |||
| onRequesterPaysError | |||
| }, _ref) => { | |||
| const { ExportAnalysisModal } = analysisComponentDependencies.get() | |||
There was a problem hiding this comment.
minimal edge adaptation for rest of codebase that is not ready to fully participate in Composable DI framework.
| @@ -249,6 +249,8 @@ const PreviewHeader = ({ | |||
| queryParams, runtime, readOnlyAccess, onCreateRuntime, analysisName, currentFileToolLabel, workspace, setCreateOpen, refreshRuntimes, | |||
| workspace: { canShare, workspace: { cloudPlatform, namespace, name, bucketName, googleProject, workspaceId } } | |||
| }) => { | |||
| const { ExportAnalysisModal } = analysisComponentDependencies.get() | |||
There was a problem hiding this comment.
minimal edge adaptation for rest of codebase that is not ready to fully participate in Composable DI framework.
| const { ButtonPrimary } = deps.components | ||
| const { useAnalysisExportState } = deps.analysisState | ||
|
|
||
| const uniqueId = useUniqueId() |
There was a problem hiding this comment.
the rest of this FC code is from a pending IA team PR and not central to the Composable DI pattern being demonstrated here. You can skim down to line 127
| const useHook: AnalysesExportStateHook = (hookArgs): AnalysisExportState => { | ||
| const { sourceWorkspace, printName, toolLabel } = hookArgs | ||
| const { AnalysisProvider } = deps.analysisProviders | ||
| const { captureEvent } = useMetricsEvent() |
There was a problem hiding this comment.
the rest of the hook code here can be skimmed past to line 114. It is from a pending IA PR and not central to the Composable DI patterns being demontrated. It does however show a couple of examples of using LoadedData and useLoadedData helper hook for those who are curious.
|
Note that this PR does not demonstrate the proposed project-level directory re-organization proposal. |
| compose: (deps: ExportAnalysisModalDeps) => { | ||
| const FC: React.FC<ExportAnalysisModalProps> = props => { | ||
| const { fromLauncher, onDismiss, printName, toolLabel, workspace: sourceWorkspace } = props | ||
| const { ButtonPrimary } = deps.components |
There was a problem hiding this comment.
My understanding is that code like this and const { ExportAnalysisModal } = analysisComponentDependencies.get() takes the place of how we would usually import a component when we want to render it... depending on whether the component you're working in is or is not participating in the DI framework.
I see the developer experience around this as a hurdle to understanding how to work in the codebase. Knowing not to just import ExportAnalysisModal and instead use one of the other ways of obtaining a handle to the component (depending on what context you're in) will have a highly variable learning curve for developers.
IDEs won't help with this, and can even hurt. If I know that the name of the component I want to render is ExportAnalysisModal and I type ExportAnalysisModal where it isn't yet defined, my IDE is (probably) going to suggest importing the thing of type ComposableExportAnalysisModal. This would be incorrect and the developer would need to know how to manually fix this. (The effect gets compounded when copying/pasting from one place to another where the IDE might try to import multiple things automatically that would need manual fixing.)
I wonder a few things:
- Is there any way to trick the IDE into giving the correct help?
- Can
ExportAnalysisModalbe given a slightly different name -- something that indicates that it is an injectable -- with the effect being that there isn't anything namedExportAnalysisModalto import? This would at least keep the IDE from unhelpfully importing the wrong thing, and suggest to the developer that there's more going on here. - How does this compare to the developer experience of some of the other options.
There was a problem hiding this comment.
an excellent concern to point out Brian...
I wonder how this hurdle would be avoided in ANY DI-Container based solution. The point of DI-Container pattern is to in fact favor the interface-defined dependency indirection... Not sure if we can make this rock-solid error-proof in any variation...
I favored keeping the naming of exports and resulting dependencies the same in this design to have less things to name, and to help in code crawling/jumping-around that is inherent in the DI indirection setup.
While this PoC tries to make things explicit and as obvious as possible, there are no guard-rails enforcing error-free pattern replication/adherence. I've learned this is generally not worth the effort versus loss in long-term design adaptability. Exploring an InversifyJS based implementation would probably land us in a similar situation: nicely obvious "I am injecting these things", but no hard enforcement.
This draft PR is NOT INTENDED for merging. It is mean to drive discussion on key aspects of the DI and related proposals in
DSP Cross-Team UI Code Sharing