Datahub as a microfrontend host (react) #15358
Conversation
github-actions
bot
added
product
Bundle ReportChanges will increase total bundle size by 49.24kB (0.17%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: datahub-react-web-esmAssets Changed:
Files in
|
datahub-cyborg
bot
added
the
needs-review
philip-nikolov-jpmc
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
ani-malgari
left a comment
ani-malgari
left a comment
There was a problem hiding this comment.
This is amazing! Thank you for unlocking MFEs. I left some minor comments.
| '/api/v2/graphql': frontendProxy, | ||
| '/openapi/v1/tracking/track': frontendProxy, | ||
| '/openapi/v1/files': frontendProxy, | ||
| '/api/mfe/config': frontendProxy, |
There was a problem hiding this comment.
this needs to be changed per feedback from Chris on the other PR.
| throw new Error('[MFE Loader] Invalid YAML: missing microFrontends array'); | ||
| } | ||
| // Validate each entry, keeping both valid and invalid ones | ||
| parsed.microFrontends = parsed.microFrontends.map(validateMFEConfig); |
There was a problem hiding this comment.
not a big deal but we can filter out the invalid ones instead of storing them.
| console.log('[HOST] attempting mount'); | ||
|
|
||
| // Parse module string, something like: "myapp/mount" | ||
| const [remoteName, modulePath] = module.split('/'); |
There was a problem hiding this comment.
since we expect exactly 1 / in the module, i think it's worthwhile to also validate in the configLoader and also add a comment in the MFEConfig type
| // MFE section (dropdown or spread) | ||
| let mfeSection: any[] = []; | ||
| if (mfeConfig) { | ||
| if (mfeConfig.subNavigationMode) { |
There was a problem hiding this comment.
this is great. thanks for accommodating subNavigation
|
|
||
| export function useDynamicRoutes(): JSX.Element[] { | ||
| const mfeConfig = useMFEConfigFromBackend(); | ||
| if (!mfeConfig) return []; |
There was a problem hiding this comment.
we could also add loading states for better UX.
| @@ -0,0 +1,191 @@ | |||
| import React, { useEffect, useRef, useState } from 'react'; | |||
There was a problem hiding this comment.
also it'll be great if we could cleanup the logs and performance monitoring that's not necessary.
| const mfeConfig = useMFEConfigFromBackend(); | ||
| if (!mfeConfig) return []; | ||
| // TODO- Reintroduce useMemo() hook here. Make it work with getting yaml from api as a react hook. | ||
| const isValidMFEConfig = (entry: MFEConfigEntry): entry is MFEConfig => !('invalid' in entry && entry.invalid); |
There was a problem hiding this comment.
Thanks for adding the TODO. Let us know if you cannot get to it, i think it's important to wrap it in useMemo instead of computing the routes on every load.
datahub-cyborg
bot
added
pending-submitter-response
3 participants
No description provided.