-
Notifications
You must be signed in to change notification settings - Fork 26
STRIPES-861 - integration of module federation logic with main branch. #1679
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…ding of ui-elements
Draft: load translations when loading remote modules Note: QueryClientProvider must be explicitly shared See https://tanstack.com/query/v3/docs/react/reference/QueryClientProvider Refs STCOR-718, STRIPES-861
Load remote icons, and clean up the translation loading a bit; it was still very much in draft form, and still is, but at least it doesn't throw lint errors everywhere now. Refs STCOR-725, STRIPES-861
Correctly set each apps' localized `displayName` attribute. It isn't totally clear to me why this doesn't work via `formattedMessage`. It seems that something is happening asynchronously that we don't realize is async, and therefore don't await, and then we end up calling `formatMessage()` before the translations have been pushed to the store. In any case, pulling the value straight from the translations array works fine. Refs STCOR-718
Correctly handle multiple icons per application. Refs STCOR-725
Major refactoring in stripes-core between this branch's initial work and the present lead to some discrepancies. The only change of note here, I think, is the relocation of `<Suspense>` from ModuleRoutes down into AppRoutes. It isn't clear to me why that was necessary or why it worked. It was just a hunch that I tried ... and it worked. Prior to that change, AppRoutes would get stuck in a render loop, infinitely reloading (yes, even the memoized functions). I don't have a good explanation for the bug or the fix.
Jest Unit Test Results 1 files ± 0 84 suites +1 1m 45s ⏱️ +3s For more details on these failures, see this check. Results for commit 4282ea8. ± Comparison against base commit 5d4fda8. This pull request removes 1 and adds 26 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Bigtest Unit Test Results152 tests +2 149 ✅ ±0 6s ⏱️ ±0s For more details on these failures, see this check. Results for commit 4282ea8. ± Comparison against base commit 5d4fda8. This pull request removes 152 and adds 152 tests. Note that renamed tests count towards both.This pull request removes 1 skipped test and adds 1 skipped test. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
zburke
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This loosk good overall, though I can't get it working with a monolithic stripes.config.js so we need to investigate that before we merge.
Nothing jumps out at me beyond the need to clean up comments and async/await a tiny bit. Since mod-fed hinges on the presence of okapi.entitlementUrl, don't we need a check like
if (okapi.entitlementUrl === okapi.url) { ... } // production
else { ... } // development
to indicate whether to use the entitlement-registry passed into stripes.config.js (dev) or just to grab stuff from the regular discovery request (production)?
Let's talk about this part of things... I don't know enough about discovery to confirm this as a assured case. |
zburke
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙌 with this branch I can now serve a federated build, serve a monolith, and build a monolith or code-split bundle. Sweet! 👏👏👏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
monkey <<< I cannot explain that except that I typed in some junk text while testing https://github.com/orgs/community/discussions/183653 and the value persisted even though it didn't display in the UI
| * @param {array} remotes | ||
| * @returns {app: [], plugin: [], settings: [], handler: []} | ||
| */ | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
omit this extra newline
| * settings, handler) where the value of each is an array of corresponding | ||
| * applications. | ||
| * | ||
| * @param {array} remotes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Document all the arguments. What is the shape of an entry in the remotes array? Someday, this'll all be TS, won't it?
| actsAs.forEach(type => modules[type].push({ ...remote })); | ||
| }); | ||
| } catch (e) { | ||
| stripes.logger.log('core', `Error preloading modules from entitlement response: ${e}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need stripes for anything other than logging and if so, is it worth it? There is only a single catch-clause here; it's not like we're handling errors inside the loop and carrying on with 99 applications if there was only an error in 1. If there's an error, everything stops. Is logging this through stripes a good-enough way to handle this kind of error, or should we throw it up a level, log it via console.error etc?
| */ | ||
| const loadIcons = (stripes, module) => { | ||
| if (module.icons?.length) { | ||
| stripes.logger.log('core', `loading icons for ${module.module}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this was probably my code ... but now I'm wondering if we want to log verbose stuff like "Hola! I found an icon!" in a category like core that has been enabled by default since the dawn of FOLIO. Change the category? Skip logging altogether?
| setRemoteModules(cachedModules); | ||
| }; | ||
|
|
||
| fetchRegistry(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
await fetchRegistry()?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The useEffect() handler itself is not async, but the fetchRegistry() function is, so all of the async/await is within that defined function.
| // eslint-disable-next-line no-undef | ||
| await container.init(__webpack_share_scopes__.default); | ||
|
|
||
| const factory = await container.get('./MainEntry'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's cross-reference this with the corresponding entry in stripes-webpack/webpack.config.federate.remote. Even just a comment that says "the black magic here has corresponding black magic there" would be helpful. Otherwise, magic-strings like MainEntry remain, uhhhhhhm, magical.
| // if stripes-core is served from a different origin (module-federation) then | ||
| // we need to fetch translations from that origin as well rather than a relative path. | ||
| // const stripsesCoreOrigin = 'http://localhost:3000'; | ||
| // const translationUrl = new URL(translationName, stripsesCoreOrigin); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment confuses me. Here, we are loading translations from the host, right? Let's just say that. Can we remove the commented out lines?
src/components/EntitlementLoader.js
Outdated
| // if platform is configured for module federation, read the list of registered apps from <fill in source of truth> | ||
| // localstorage, okapi, direct call to registry endpoint? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where do you think this "figure out the source of truth" logic belongs -- in loadEntitlement() itself?
- in production, it'll be localstorage (having been populated during session init)
- in development, it'll be a direct API call to the registry
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both.. if not localstorage, then the local registry or whatever 'entitlementUrl' is provided... There has to be an 'entitlementUrl' in the config at build-time to even reach this logic... but this is worth a short discussion/question - will host-app builds necessarily *always involve an entitlementUrl since stripes-hub is the thing that actually uses it/cares about it the most? In this current state, stripes-core cares about the entitlementUrl, and it can work for setups that may *not use stripes-hub.
src/components/EntitlementLoader.js
Outdated
| // read the list of registered apps | ||
| let remotes; | ||
| try { | ||
| remotes = await loadEntitlement(okapi.entitlementUrl); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than pulling okapi from the stripes-config import, should we instead be pulling it directly off stripes, which is available to this component via useStripes()?
| // register sounds | ||
| // TODO loadSounds(stripes, module); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've lost track of where this work stands. There is handling for sounds in your folio-org/stripes-webpack#173 PR, so should we have corresponding code here, too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The limited case of sound is - with code in the current state - leaning on the federated app (check-out) falling back to its own optionalDependencies - and loading sound from there if the logic gets to that particular path. check-out uses a dynamic require for pulling sounds from its optionalDependency- @folio/circulation - we * could do something with what a ui-module exposes like globbing together some 'assets' and mapping out entries to the ModuleFederationPlugin's exposes config... but the case is so obscure at the moment and nothing is broken in a federated build with this. There's other cases for this kind of provision - like dashboard importing components from other ui-modules. It's a spike-able aspect to reach beyond 'sounds' and just provide a means for a ui-module to expose bits of code, components, assets, whatever to other ui-modules.
…discovery results/pull discovery results from localForage
|


This PR takes the logic from #105 and implements backward compatibility/opt-in behavior to module federation. Huge thanks to @mkuklis so long ago for his initial lift!
This works with the Stripes-webpack PR and STRIPES-CLI PR to run a federated ui platform.
The changes in this PR implement an
<EntitlementLoader>component that, provided anentitlementUrlviastripes-config.okapi, will fetch its list of modules from a dynamic source containing URL's for those module bundles. The bundles are then prefetched individually, translations, icons, loaded. The component passes through any modules provided via the monolithic buildstripes-configsetup.Adds
addIconand adjustssetTranslationsreducers to accumulate translations from async remote modules (loaded all together up front rather than on-demand at instanciation points such as AppRoutes and Settings (so the synchronous syntax of those implementations remain the same.)Try the script to clone applicable branches, a few modules and set up a workspace:
Then you can:
Build a module-federation host app (from the workspace level):
Serve the host app (dev mode from the workspace level)
Build module bundle for static hosting (at the ui-module level)
Serving the federated ui-module (dev mode at the ui-module level)
Have fun! 🎉🚀🎸🤘