This repository has been archived by the owner on May 17, 2019. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add translation key instrumentation for dynamic imports #769
base: master
Are you sure you want to change the base?
Add translation key instrumentation for dynamic imports #769
Changes from 8 commits
f39d2e7
adc025a
d7cc0e1
14fe62f
7236f02
af8fb96
130dbe6
c17ab57
142a2f2
58dc442
039274b
5b8b60a
bd44167
07b757a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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:
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 asplit
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.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 inchunk._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 thatchunk._modules
wasn't working in a production build for themain
bundle. I don't fully understand it, butmodule.dependencies
works in production andchunk._modules
works in developmentThere 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 containsNormalModule
s whereas in the production build most modules (especially components that import other components) are replaced by a fewConcatenatedModule
s. In order to get the filenames from this I have to do a little digging intodep.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).