-
Notifications
You must be signed in to change notification settings - Fork 3
STRIPES-861 integration of module federation logic in the main branch. #173
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
* map the `sounds` directory for remote applications, analogous to how translations and icons are served * provide `/code` to make the registry human-readable * catch and display startup errors in case humans make stupid coding mistakes and need help finding them
Icons in stripes-components are imported as components whereas those in applications are just resources, so we need to load them differently.
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.
Most of this looks great and I am very excited to keep pushing ahead with it. That said, I have two big concerns:
- I don't understand the use-case for stripes-local-federation-plugin.
- I'm deeply concerned about hard-coding the versions of the peer-dep singletons.
Let's plan some time to talk about these. Maybe I don't understand the constraints very well, or maybe this is a great initial implementation and some of these gotchas can be mitigated with future work.
webpack/serve.js
Outdated
| // Start the local registry server if the entitlementUrl is not an absolute URL ex localhost:3001/registry | ||
| if (entitlementUrl.includes('localhost')) { | ||
| try { | ||
| registryServer.start(entitlementUrl); | ||
| } | ||
| catch (e) { | ||
| console.error(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.
This comment does not make sense to me, and it differs from the actual implementation. I think the impl is correct to only start the entitlement service on localhost, but that impl feels self-evident. Maybe we need a header comment at the top of the outer conditional explaining this at a high-level instead?
|
@zburke for posterity sake, |
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.
I have a lot of nitpicky comments, but that's all I really have. I'm still a bit confused about when it would be legit for stripes-core not to be available and we'd need to use the fallback, but the flipside is that it's great to see that we're trying to detect local stripes-core dependencies as the first approach to determining singletons and their versions.
Next step for me is to get this cloned locally alongside all the other WIP branches, make sure we can run local federated builds and generate monolithic and federate builds, and then I'll move this to approve!
| // publicPath for how remoteEntry will be accessed. | ||
| let url; | ||
| const port = options.port ?? await portfinder.getPortPromise(); | ||
| const host = options.host ?? `http://localhost`; |
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.
Petty: quotes, not backticks. Hello lint? Wherrrrre arrrrre youuuuuuu?
| // publicPath for how remoteEntry will be accessed. | ||
| let url; | ||
| const port = options.port ?? await portfinder.getPortPromise(); | ||
| const host = options.host ?? `http://localhost`; | ||
| if (options.publicPath) { | ||
| url = `${options.publicPath}/remoteEntry.js` | ||
| } else { | ||
| url = `${host}:${port}/remoteEntry.js`; | ||
| } |
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.
Comments would be helpful here, e.g. what is remoteEntry? Can/should we consider moving some of this below the if (options.build) conditional, or is it easier to leave it up here since config is the return value of a function that receives options and we don't want to have to massage that data again later?
| .catch(error => { | ||
| console.error(`Registry not found. Please check ${entitlementUrl}`); | ||
| process.exit(); | ||
| }); |
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.
Since we're using async, let's do
try { async whatever(); }
catch (err) { ... }
instead of tacking on a .catch(err => { ... }) to the promise.
| try { | ||
| await axios.delete(entitlementUrl, { data: metadata }); | ||
| await fetch(entitlementUrl, { | ||
| method: 'DELETE', | ||
| headers: requestHeader, | ||
| body: JSON.stringify(metadata), | ||
| }).catch(error => { | ||
| throw new Error(error); | ||
| }); | ||
| // await axios.delete(entitlementUrl, { data: metadata }); | ||
| } catch (error) { | ||
| console.error(`registry not found. Please check ${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.
Remove duplicate catch clause
| const app = express(); | ||
|
|
||
| app.use(express.json()); | ||
| app.use(cors()); |
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.
Sonar gets panicky about configuring cors wide-open like this. Maybe this is a case of "Shut up Sonar, this is dev-mode and we can't know what our config will be, so wide-open is perfect." Or maybe we could restrict this to localhost. Dunno; thoughts?
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.
Yes. Total case of sonar being over-protective.
consts.js
Outdated
| console.log('Unable to locate local stripes-core package.json, fetching from Github...'); | ||
| try { | ||
| const octokit = new Octokit(); | ||
| corePkg = await octokit.request('GET /repos/folio-org/stripes-core/contents/package.json', { | ||
| headers: { | ||
| accept: 'application/vnd.github.raw+json' | ||
| } | ||
| }); | ||
|
|
||
| if (corePkg.status !== 200) { | ||
| throw new Error('Error retrieving singletons list from platform. Falling back to static list'); | ||
| } | ||
| } catch (e) { | ||
| console.log(e); | ||
| return singletons; | ||
| } |
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.
On the one hand, it's nice that we're trying to be resilient. OTOH, is there a legit circumstance for a remote-app not to depend on the host-app, which is the only reason we could possible end up down here, right? IOW, should this be a recoverable error with a fall back or should it just be a fatal error?
Related, what branch/tag are these values being pulled from?
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.
Removed this fallback for now. If we ever *do determine that we need it, it's an option for dynamically getting those values.
| const defaultentitlementUrl = 'http://localhost:3001/registry'; | ||
|
|
||
| module.exports = { | ||
| defaultentitlementUrl, |
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.
petty: defaultEntitlementUrl?
| } | ||
| ] | ||
| }, | ||
| // TODO: remove this after stripes-config is gone. |
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.
What, specifically, will allow us to kill stripes-config? Can we point to a Jira that spells out the details if we don't want to spell them out here?
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.
leftover from POC... will remove.
| const configSingletons = await getHostAppSingletons(); | ||
| const shared = processShared(configSingletons, { singleton: true }); | ||
|
|
||
| const config = { |
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 feel like all these keys need some comments, or at least a pointer at the top to a particular section of the webpack manual. Otherwise, I wanna know what is publicPath vs path? I wanna know what happens if I don't provide it and it defaults to auto? I wanna know what you're thinking, tell me what's on your mind.
| { | ||
| test: /\.(woff2?)$/, | ||
| type: 'asset/resource', | ||
| generator: { | ||
| filename: './fonts/[name].[contenthash].[ext]', | ||
| }, | ||
| }, | ||
| { | ||
| test: /\.(mp3|m4a)$/, | ||
| type: 'asset/resource', | ||
| generator: { | ||
| filename: './sound/[name].[contenthash].[ext]', | ||
| }, | ||
| }, |
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.
Many of these entries are already duplicated across webpack.config.base.js and webpack.config.cli.transpile.js and now webpack.config.federate.remote.js. Is it worth trying to consolidate, or do you think that would just lead to even more config files and an even worse experience?
…k into STRIPES-861-int




This PR takes the logic from #105 and places it behind a conditional on an
okapi.entitlementUrlfield in thestripes-configobject.Further adjustment -
StripesTranslationPluginaccumulates translations from includedstripesDeps(stripes-acq-components, etc) so that those can be loaded at runtime when the dependent ui-module's translations are loaded.StripesTranslationPlugincontains a conditional where it doesn't piggyback ontostripesConfigPluginfor federated module builds.LocalFederationPluginis used for spinning up a local set of remotes of the modules via localstripes-config. It loops through the modules and spawns afederateprocess. Ifmodulesis empty, this does nothing.Conforms module federation behavior to
serve --federateandbuild --federatecommands.Will pull shared dependencies from the locally installed copy of
stripes-core, if possible, falling back to reach out tostripes-coreon github if the localstripes-corecan't be required.