-
Notifications
You must be signed in to change notification settings - Fork 86
feat(external-fallbacks): enable modules to have external fallbacks #984
Conversation
Size Change: +21.7 kB (+3%) Total Size: 710 kB
ℹ️ View Unchanged
|
595c686
to
46f41c3
Compare
b9c2af3
to
f5d90ab
Compare
@@ -29,6 +29,8 @@ export default async function initClient() { | |||
try { | |||
// eslint-disable-next-line no-underscore-dangle | |||
setModuleMap(global.__CLIENT_HOLOCRON_MODULE_MAP__); | |||
// eslint-disable-next-line no-underscore-dangle | |||
setRequiredExternalsRegistry(global.__HOLOCRON_EXTERNALS__); |
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.
NIT: for consistency with __CLIENT_HOLOCRON_MODULE_MAP__
, why not __CLIENT_HOLOCRON_EXTERNALS__
?
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.
__CLIENT_HOLOCRON_MODULE_MAP__
is the only key with the CLIENT
prefix, this is probably as the module map being set is specifically for the client and differs from the module map loaded on the server.
|
||
export const setModulesUsingExternals = (moduleNames) => { | ||
modulesUsingExternals = new ImmutableSet(moduleNames); | ||
}; |
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.
Removing parts of the old system? is this still backwards compatible and this functionality is now achieved in a different way?
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.
for the most part this has been reimplemented in holocron - https://github.com/americanexpress/holocron/pull/143/files#diff-c59d732ae8137c4f28a025d9cabd09b4fd3166326f1822eb6b9a94089d89d777R68-R77
it does appear that the call to clearModulesUsingExternals
has been removed when it should not have.
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.
Approved with 1 NIT and 1 question
ce02f90
00b529e
to
86eb04c
Compare
This PR requires release and update of Holocron with the changes outlined in americanexpress/holocron#143
Description
Enable modules which require externals to use a fallback when the root module does not provide one.
Record each modules required externals when the module is required by one-app.
When rendering html to the client include all the fallback script tags required for the modules used in the initial render.
pass the registry of required externals down to the client then hydrate when client initializes
Motivation and Context
This will allow more flexibility to holocron modules when a required external is missing. It also provides an additional upgrade path for major versions of external dependencies.
How Has This Been Tested?
Types of Changes
Checklist:
What is the Impact to Developers Using One App?