-
Notifications
You must be signed in to change notification settings - Fork 37
Add translation key instrumentation for dynamic imports #769
base: master
Are you sure you want to change the base?
Conversation
|
const modulesSet = new Set(); | ||
|
||
// Module dependencies | ||
if (dep.module && dep.module.dependencies) { |
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.
Interesting, so these modules in module.dependencies
are not already in chunk._modules
for each chunk in the chunk group?
Regardless, I think it might be worth encapsulating logic this into a separate function:
dep => Set<string>
.
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 originally didn't have to use module.dependencies
, but I learned through testing that chunk._modules
wasn't working in a production build for the main
bundle. I don't fully understand it, but module.dependencies
works in production and chunk._modules
works in development
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 think I can elaborate on this a little more. In dev, chunk._modules
only contains NormalModule
s whereas in the production build most modules (especially components that import other components) are replaced by a few ConcatenatedModule
s. In order to get the filenames from this I have to do a little digging into dep.module.dependencies
. This makes the set of filenames larger than it probably has to be, but it also seems to be accurate (i.e. contains files we need and not ones we don't need).
build/plugins/instrumented-import-dependency-template-plugin.js
Outdated
Show resolved
Hide resolved
build/plugins/instrumented-import-dependency-template-plugin.js
Outdated
Show resolved
Hide resolved
Co-Authored-By: micburks <[email protected]>
Co-Authored-By: micburks <[email protected]>
build/get-webpack-config.js
Outdated
* The underlying plugin is able determine client chunk metadata on its own. | ||
*/ | ||
new InstrumentedImportDependencyTemplatePlugin( | ||
void 0, |
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.
Now that this constructor now takes more than one parameter, I think an object might make this more understandable. This was already a bit confusing (my bad) where the argument is expected to be undefined in the client case, but server-side should be provided. Adding another parameter now makes it even worse.
Expressing the parameter type as a disjoint union of two different objects would help I think. For example:
type Opts =
| ClientOpts
| ServerOpts;
type ServerOpts = {
compilation: "server",
clientChunkMetadata: ClientChunkMetadataState
};
type ClientOpts = {
compilation: "client",
i18nManifest: TranslationsManifest
};
Also, this makes me think: is it odd that __I18N_KEYS
is only added on the client-side? The other two properties are added in both the server and client bundles.
I think in the future, potentially we could align the server code to work more like the client code. I think this particular promise instrumentation might be useful even in a Suspense-powered i18n system.
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.
Yeah I think that'll make the plugin easier to understand. I'll update to use those types.
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 was actually thinking the opposite for the promise instrumentation. I don't totally understand why __CHUNK_IDS
are added to the server build since they're only used when dynamically loading chunks with new translations (at least I think that's the only time they're used).
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.
__CHUNK_IDS
is necessary server-side because that's how the server knows which async chunks are used in the SSR and are thus considered "critical" and should therefore be preloaded. During SSR, when it encounters a split
component, it adds the chunk ids (on the promise object) to the list of critical chunks that should be preloaded.
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.
Okay I recall that now. Since I didn't change how the translation manifest is added to the server bundle, the server still looks up the necessary translations based on the __CHUNK_IDS
of the promise. We could potentially change that to use use __I18N_KEYS
but yes probably not really necessary at this point. It might require a different approach than what I have because this approach doesn't find any translation keys since they're all bundled together in the server build.
Annotate dynamic imports with the translation keys the chunk contains
Fixes issues with dynamically loading translations where routing isn't guaranteed to hit the origin the initial request was served from. This will allow looking up a translation by key instead of the id of the chunk that contains it.
Essentially I had to split the i18nManifest state into its
DeferredState
and theMap
that it resolves to. This way, the server build can wait for the client build to finish (as before) by referencing theDeferredState
and the instrumentation plugin can use theMap
directly to lookup translation keys.This is supplemented by PRs in
fusion-react
,fusion-plugin-i18n
, andfusion-plugin-i18n-react
.