Skip to content
This repository has been archived by the owner on May 17, 2019. It is now read-only.

Add translation key instrumentation for dynamic imports #769

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

micburks
Copy link
Contributor

@micburks micburks commented May 3, 2019

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 the Map that it resolves to. This way, the server build can wait for the client build to finish (as before) by referencing the DeferredState and the instrumentation plugin can use the Map directly to lookup translation keys.

This is supplemented by PRs in fusion-react, fusion-plugin-i18n, and fusion-plugin-i18n-react.

@micburks
Copy link
Contributor Author

micburks commented May 3, 2019

PR title is invalid: First word must be imperative verb.

Instrument can be an imperative verb

@micburks micburks changed the title Instrument dynamic imports with translation keys Add translation key instrumentation for dynamic imports May 3, 2019
@micburks micburks added the bugfix label May 3, 2019
@micburks micburks requested a review from rtsao May 8, 2019 16:46
lhorie
lhorie previously approved these changes May 8, 2019
const modulesSet = new Set();

// Module dependencies
if (dep.module && dep.module.dependencies) {
Copy link
Member

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>.

Copy link
Contributor Author

@micburks micburks May 8, 2019

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

Copy link
Contributor Author

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 NormalModules whereas in the production build most modules (especially components that import other components) are replaced by a few ConcatenatedModules. 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/get-webpack-config.js Outdated Show resolved Hide resolved
* The underlying plugin is able determine client chunk metadata on its own.
*/
new InstrumentedImportDependencyTemplatePlugin(
void 0,
Copy link
Member

@rtsao rtsao May 8, 2019

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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).

Copy link
Member

@rtsao rtsao May 9, 2019

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.

Copy link
Contributor Author

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants