Opened 6 years ago

Last modified 2 years ago

#17155 assigned defect

The content of a flattened _ROOT nls bundle is not taken into account

Reported by: Patrick Ruzand Owned by: Colin Snover
Priority: blocker Milestone: 1.15
Component: Loader Version: 1.9.0
Keywords: Cc: dg, Adam Peller, Clement Mathieu
Blocked By: Blocking:

Description

We found something very strange regarding how dojo treat nls modules included in a _Root flattened bundle compare to the case of _<locale> flattened bundles. For example, my dojo build does not have "en" or "en_us" included in the generated locales. So, when the page loads up with "en" or "en_us" locale, dojo will fallback to load the _Root flattened bundle for the nls. Compare to other locale, e.g., fr, which is one of the generated locales, when page loads up with "fr" locale, dojo will load the _fr flattened bundle, and not the individual files. However, whenever dojo falls back to use the _Root nls flattened bundle, it does not seem to recognize the nls modules inside the layer _ROOT, in fact, it tries to load individual nls file again.

From what I see in the dojo/i18n code, the problem seems to be due to the fact that, in the case the current locale is not in the list of the preloaded locales, the i18n plugin will load the _ROOT bundle, and a root bundle contained in a flattened bundle is cached in the loader cache with a mid like "myapp/nls/mybundle/ROOT" (done in the i18n.preload() function). But then, when a require for this bundle is done, (e.g requiredojo/i18n!myapp/nls/mybundle?), the i18n plugin will falls back to its "new 1.7 default" behavior implemented in doLoad(). Which means, it first requires the root bundle before iterating over the possible locales:

doLoad = function(require, bundlePathAndName, bundlePath, bundleName, locale, load){
  require([bundlePathAndName], function(root){
    var current = lang.clone(root.root),
	availableLocales = getAvailableLocales(!root._v1x && root, locale, bundlePath, bundleName);
    require(availableLocales, function(){
[...]
}

The problem here is that the root bundle is loaded using its "original" mid (aka "myapp/nls/mybundle") instead of the one used during the preload phase (aka "myapp/nls/bundle/ROOT").

To summarize, it seems to me there's an inconsistency in the ways a root bundle is identified when it is loaded via a preloaded flattened bundle (cached using the "myapp/nls/bundle/ROOT" id) and when it is loaded during the fallback path in doLoad().

Attachments (10)

testBuild.zip (4.7 KB) - added by Patrick Ruzand 5 years ago.
testcase. the dojo src is assumed to be in scripts/dtk.
i18n.patch (6.4 KB) - added by Clement Mathieu 5 years ago.
writeAmd.js (11.6 KB) - added by Rawld Gill 5 years ago.
include all available localizations in a flattened root bundle
i18n.js (22.3 KB) - added by Rawld Gill 5 years ago.
cause flattened root bundle to be understood loaded by the loader
i18n.2.js (23.3 KB) - added by Clement Mathieu 5 years ago.
i18n with rawld patch and less specifique fallback.
writeAmd.2.js (12.0 KB) - added by Rawld Gill 5 years ago.
annotate flattened bundles to say what other more-specific locales are available
i18n.3.js (24.6 KB) - added by Rawld Gill 5 years ago.
improve preloading to ensure the best locale is loaded for each bundle even if it wasn't flattened
ticket17155.zip (12.4 KB) - added by Rawld Gill 5 years ago.
extensive test
regressionCase.zip (164.8 KB) - added by Clement Mathieu 5 years ago.
Test case for the regression introduced by fix for #17155
regressionCase2.zip (1.2 KB) - added by Clement Mathieu 5 years ago.
Test case for the second regression.

Download all attachments as: .zip

Change History (47)

comment:1 Changed 6 years ago by Patrick Ruzand

Component: InternationalizationLoader
Owner: changed from Adam Peller to Rawld Gill

Changed Component to "Loader" as it seems it's more a i18n/loader/build issue, so I guess should be in the rawld's hands. (Feel free to change if I am wrong).

comment:2 Changed 6 years ago by Patrick Ruzand

Milestone: tbd1.9.1
Priority: undecidedhigh

comment:3 Changed 6 years ago by Patrick Ruzand

I raised the priority as I think it's really a critical issue: it forces the deployment of the individual root bundles while the goal of the flattened bundle was to avoid this.

comment:4 Changed 6 years ago by cjolif

Cc: dg added

comment:5 Changed 6 years ago by cjolif

FYI this is hurting one of our product badly.

comment:6 Changed 5 years ago by Adam Peller

Cc: Adam Peller added

comment:7 Changed 5 years ago by Colin Snover

Milestone: 1.9.11.9.2

1.9.1 is released; moving all outstanding tickets to next milestone.

comment:8 Changed 5 years ago by cjolif

This was reported one more time to me today. Any chance to get it fixed?

comment:9 Changed 5 years ago by cjolif

Cc: Clement Mathieu added

Changed 5 years ago by Patrick Ruzand

Attachment: testBuild.zip added

testcase. the dojo src is assumed to be in scripts/dtk.

comment:10 Changed 5 years ago by Patrick Ruzand

The above testcase reproduces the issue. The build profile is located in <install-dir>/scripts. The dojo src is assumed to be in <install-dit>/scripts/dtk. The app html is <install-dir>/demo-release17-build.html, and it displays a message in the console. The build is run using the following command line:

node ..\..\dojo\dojo.js load=build --profile ..\..\..\demo.profile.js

The <install-dir>/demo-release17-build.html forces the locale to "aa", that is, a locale for which there's no flattened bundle, and therefore, the fallback mechanism occurs and the flattened ROOT bundle is loaded. If you look at the Network traffic in the debugger, one can notice the following files are loaded: /dojo/testbuild/demo-release17-build.html /dojo/testbuild/release/dojo/dojo.js.uncompressed.js /dojo/testbuild/release/dojo/nls/dojo_ROOT.js /dojo/testbuild/release/myapp/src.js /dojo/testbuild/release/myapp/nls/src_ROOT.js /dojo/testbuild/release/myapp/nls/greetings.js /dojo/testbuild/release/dojo/cldr/nls/number.js /dojo/testbuild/release/dojo/cldr/nls/currency.js

which means the individuals nls modules (myapp/nls/greetings, dojo/cldr/nls/number and dojo/cldr/nls/currency) are loaded individually while they are already included in the src_ROOT.js flattened bundled.

If you force the locale to "en" (for which there's a flattened bundle), then it works as expected.

comment:11 Changed 5 years ago by Clement Mathieu

After some investigations, I think this bug is coming from this part of the i18n plugin (around line 275) :

	targetLocale =	localeSpecified || dojo.locale || "",
	loadList = localeSpecified ? [targetLocale] : getLocalesToLoad(targetLocale),
	
	[...]
	
	array.forEach(loadList, function(locale){
		var target = bundlePathAndName + "/" + locale;
		if(!cache[target]){
			doLoad(require, bundlePathAndName, bundlePath, bundleName, locale, finish);
		}
	}

During this step, the code is testing if the bundle corresponding to the targeted locale is in the i18n cache and if not, it loads the bundle via a require() (the doLoad method), The problem is, when flattened bundles are in use, that the target locale may not be a generated locale. So, in such case (flattened bundle + locale that is not in the localeList ), when the plugin is looking for the target locale bundle in its cache, it does not find it (logical, since the locale does not match a preloaded flattened bundle), and ends up requiring the module using the new 1.7 amd way (loading the individuals nls modules). To fix that bug, it should test the cache with the real used locale (that is, if flattened bundles are in use, the generated locale that best matches the target locale, or "ROOT" if none) instead of the targeted locale.

I have attached a "patch" to demonstrate where the problem appears and a possible approach to fix it following the methodology described above.

Changed 5 years ago by Clement Mathieu

Attachment: i18n.patch added

comment:12 Changed 5 years ago by Rawld Gill

I generally agree with cmathieu's analysis. Let me say what's happening is slightly different words:

1. When preloads are present, loading a layer will cause a preload of the best bundle given the current locale.

2. If the current locale is is not mentioned in the preloads, then the best is "ROOT" and that flattened bundle is loaded; a "flattened root" bundle is not equivalent to a typical root bundle since a flattened root bundle does not include the hash of available locales.

3. When a bundle is then required by a normal module, the i18n machinery will fail to find bundle and proceed with the normal i18n loading sequence...which causes the normal (i.e., not flattened) root bundle to be loaded, and then the locale-specific bundle of available.

I think this is not fixable solely by patching the i18n module. Consider the case where, in the example provided by pruzand, the locale aa does exist, but was not flattened. Then, ideally, the root should be loaded by the preload algorithm and followed by the aa bundle using the normal algorithm.

I believe the fix should be two-fold:

  1. Include the available locales in the flattened root bundle (change to builder).
  2. Cache the preloaded root (change to i18n).

I think these are both micro changes. I'll proposed a patch today.

comment:13 Changed 5 years ago by Clement Mathieu

I have made two pull request on GitHub? with a patch to fix that bug. Following rcgill proposition, the patch is two-fold so there is two pull request.

They can be reviewed here:

comment:14 Changed 5 years ago by Rawld Gill

Status: newassigned

I think the proposed patch above is not the best solution for the following reasons:

  1. The line

https://github.com/clmath/util/blob/bbba18cf8d6e009e9be74232aa515cc1acc2362b/build/transforms/writeAmd.js#L212

causes the preload command to instruct the i18n module that *every* bundle has *every* locale mentioned in *any* bundle. This may not be the case.

  1. There is some expressive inefficiency. For example, the function forEachLocale (https://github.com/clmath/dojo/blob/98ffecc49ce110c8f802ae0a13f27a9c13cd75eb/i18n.js#L40) is already mentioned in the i18hn module...why write it twice?

I still believe my proposed solution is the way forward (see attached files). However, cmathieu noted that if a locale is requested (e.g., en-us) for which there is a less-specific flattened version, then the flattened resources are ignored.

I'm not sure that is really worth worrying about. Considering the example above...if en-us is so important that the normal load sequence is offensive, then simply add that locale to the locale list during the build.

The alternative implies yet more complexity. Continuing the example with en-us, we must notate the flatted en-bundle to say either that a specialized en-us is available (or not), or when trying to preload en-us and noticing that only a less specific bundle is available, causing the root to also be preloaded. This complexity for very little if any performance gain seems foolish.

Interested folks should provide a counter argument.

I'm attaching my proposed change.

Changed 5 years ago by Rawld Gill

Attachment: writeAmd.js added

include all available localizations in a flattened root bundle

Changed 5 years ago by Rawld Gill

Attachment: i18n.js added

cause flattened root bundle to be understood loaded by the loader

comment:15 Changed 5 years ago by Clement Mathieu

Thank you rcgill for your feedback on my proposition, and I agree this is not the best solution.


However, I am still convinced that the case where there is a less-specific flattened bundle for the locale requested (e.g., flatten exist for en when en-us is requested) worth being solved.

Here are my counter arguments:

  • Adding the locale to the localeList cannot be a good solution as an unflattened locale will be requested at some points.
  • The performance gain is huge if the user is on a high-latency connection. Currently, when accessing an application with only 10 nls bundles (which is a very reasonable number), for locale en-us and with en and ROOT flatten bundle available, dojo will download :
    • the en flatten bundle (1 http request)
    • all the individual root bundles (10 http requests)
    • all available individual en bundles (around 10 http requests)
    • all available individual en-us bundles (I don't count those ones as they have to be downloaded anyway)

So there is 19 unnecessary http requests and that is if we only have 10 nls modules. With an average latency of 400ms on a 3G network, doing those requests from a smartphone has a real impact on performance.


Considering this, I have started with your patch, and added support for that functionality. As you suggested in your comment, I changed the preloadL10n function to preload the root flatten bundle if there was no flatten bundle for the requested locale. I have also changed the getAvailableLocales function so it stops to enumerate bundles when one is in cache.

For example, assuming:

  • the root bundle indicates specific bundles for "fr" and "fr-ca",
  • bundlePath is "myPackage/nls"
  • bundleName is "myBundle"
  • myPackage/nls/myBundle/fr is already in cache

Then a locale argument of "fr-ca" would return ["myPackage/nls/fr/myBundle", "myPackage/nls/fr-ca/myBundle"]

This way the i18n plugin is not doing unnecessary requests and the example from my counter argument above will do only 2 http requests (one for root flatten bundle and one for en flatten bundle). It also saves a few computations as the bundle from cache has already been mixed-in with the root bundle.


I would greatly appreciate if you had time to look at my proposition and give me your feedback.

You can review my modifications on github or in the attached file. https://github.com/clmath/dojo/compare/17155-rawld...17155-me

Changed 5 years ago by Clement Mathieu

Attachment: i18n.2.js added

i18n with rawld patch and less specifique fallback.

comment:16 Changed 5 years ago by Clement Mathieu

Since there was a lot of discussion on this bug, I will try to summarize it before presenting my proposed fix.

Setup

Let's assume the following setup, with the user locale set to fr-fr and the list of flattened locales being [fr, root]:

myapp/
  |_nls/
    |_fr-fr/
      |_greetings.js
    |_fr/
      |_greetings.js
    |_greetings.js
    |_src_fr.js
    |_src_ROOT.js
  |_src.js

myapp/src.js is the main app layer and myapp/nls/src_fr.js is the corresponding flattened nls layer for the locale fr.

The bug

The bug was first reported appearing when a user requested a non existing locale like foo. Since there is no flattened nls layer for foo, the i18n preload system loads the flattened nls layer with the closest match to the required locale (myapp/nls/src_ROOT.js in that case). Then, the app will require the greetings bundle for the locale foo. Since the bundle is not in cache, the plugin switches to "unbuilt" mode, hence requiring the individual root bundle, even though the ROOT nls layer has already been preloaded. Further investigation has shown that the issue also appears when a more specific locale is required (e.g. fr-fr) and only a less specific locale has been flattened (e.g. fr). In this case the individual fr bundles are loaded, even though the fr nls layer has been preloaded.

rcgill proposal

rcgill submitted a fix solving the problem for the ROOT fallback issue only.

When a user is requiring a non existing locale (e.g. aa), the i18n preload system will load the root flattened nls layer. When a bundle will be required by the application, the i18n plugin will look for the bundle with locale aa in its cache and will not found it (since only the root layer was preloaded). So the system also switches to "unbuilt" mode and requires the individual root bundle.

To fix this, rcgill proposal is to add the list of available locales for each bundle in the root flattened layer. This way the loading of the individual root bundle can be avoided.

nfortunately this fix does not fix the more general case of the fallback to less-specific locale.

My proposal

Since rcgill fix is not completely solving the issue, I enhanced it to account for the general case.

I have made two changes on top of rcgill patch:

  1. If the flattened nls layer is not a *perfect* match (e.g. there is a flattened nls layer for fr but not for fr-fr), the preload system will also preload the ROOT layer as it will be useful later to determine for which locale there's a specific bundle.
  1. When requiring a bundle, the i18n plugin will check if the module is in cache not only for the required locale (e.g. fr-fr) but also for all the fallback locales (e.g. fr and ROOT). This way, the system avoids to download the individual fr bundle that was preloaded in the fr nls layer.

Changed 5 years ago by Rawld Gill

Attachment: writeAmd.2.js added

annotate flattened bundles to say what other more-specific locales are available

Changed 5 years ago by Rawld Gill

Attachment: i18n.3.js added

improve preloading to ensure the best locale is loaded for each bundle even if it wasn't flattened

comment:17 Changed 5 years ago by Rawld Gill

I think there is a better way that requires fewer changes (in the attached proposal, notice all changes are localized to the preload loop).

During build time, annotate each flattened bundle with the list of more-specific locales that are available that were not flattened. For example, if the localizations for "fr", "fr-fr", and "fr-ca" all exist, but only "fr" and "fr-fr" were flattened, then notate in the "fr" bundle that "fr-ca" is available.

Now at runtime, assume there is a demand to load this bundle (that is, this bundle is part of a flattened bundle) with the run-time locale==="fr-ca". Then the preload algorithm will fall back and load "fr". Upon doing so, it will see "fr-ca" is available and since that is the desired locale, load "fr-ca" and mix it into the already-loaded "fr" locale.

This is automatic for every bundle mentioned in a flattened bundle (remember, "flattening" does two things: (1) aggregates specific bundles at build-time (for example, the "fr-fr" bundle will be root + "fr" + "fr-fr", and (2) aggregates all of the bundles required for a particular layer in a single module). So, when we make the decision to preload, we're also making the decision to automatically load the best available bundle given the run-time locale for every bundle mentioned in the rollup.

See attached proposed code above. It has not been tested (or even run) yet. Interested folks are encouraged to give it a try

comment:18 Changed 5 years ago by Rawld Gill

Proposal debugged and tested. Test attached below. See pull requests: https://github.com/dojo/dojo/pull/41 and https://github.com/dojo/util/pull/13

Changed 5 years ago by Rawld Gill

Attachment: ticket17155.zip added

extensive test

comment:19 Changed 5 years ago by Colin Snover

Milestone: 1.9.21.9.3

comment:20 Changed 5 years ago by Patrick Ruzand <pruzand@…>

Resolution: fixed
Status: assignedclosed

In 341e41f1797739449d883e8951cda54bde53417e/dojo:

Error: Processor CommitTicketReference failed
Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 

comment:21 Changed 5 years ago by Patrick Ruzand <pruzand@…>

In 33a471c3cbd9dbe878d257a9bdb8b507721ce52c/dojo:

Error: Processor CommitTicketReference failed
Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 

comment:22 Changed 5 years ago by Patrick Ruzand <pruzand@…>

In d76bd85648251308483b9148b6389cb363e988f9/dojo:

Error: Processor CommitTicketReference failed
Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 

comment:23 Changed 5 years ago by Clement Mathieu

This fix introduces a regression. It prevents the loading of 1.6 i18n layers.

At some point in the new code there is a null pointer exception when preloading 1.6 i18n layers. I proposed an easy fix in PR#56 on github.

To reproduce the bug, download the test case and navigate to ticket17155/test17155.html.

Last edited 5 years ago by Clement Mathieu (previous) (diff)

Changed 5 years ago by Clement Mathieu

Attachment: regressionCase.zip added

Test case for the regression introduced by fix for #17155

comment:24 Changed 5 years ago by cjolif

Resolution: fixed
Status: closedreopened

comment:25 Changed 5 years ago by cjolif

Resolution: fixed
Status: reopenedclosed

regression fixed in all targeted versions. See: https://github.com/dojo/dojo/commit/b2fa879954860e6669258eee2a25c5e4432b86d3 for master.

comment:26 Changed 5 years ago by jandockx

Something still seems amiss with 1.9.3.

We just spend a day trying to understand a weird issue, introduced by 1.9.3. We do not understand as of yet exactly what happens, nor the issue discussed above.

What we see now is:

Without a localeList in the build profile, for a layer xxx/main.js, we get xxx/nls/main_xx.js files for a number of languages, a/o en-us, and fr-fr, but not en nor fr. We presume this is because localeList gets a default value that contains these languages.

When we add en and fr to dojoConfig.extraLocale, things go wrong when using i18n.getLocalization. The bundle we get for "en", or "fr", e.g., is the root bundle, and it contains no entries for "en" or "fr", resp..

We assume this has to do with preloading: no main_en or main_fr exists, and when we ask for en or fr, we get what was preloaded, which is wrong.

When we remove dojoConfig.extraLocale, things work. When we add localeList with en and fr to the build profile, things work.

In conclusion, we think these 2 need to be in sync since 1.9.3. Actually, this might be kind of a bug. If en or fr is not built, but en-us and fr-fr are, the loader should not assume that there is no "en" or "fr".

All in all, the default for localeList would better be [], to not get unexpected behaviour.

Note that our understanding of what is happening might not be correct.

comment:27 Changed 5 years ago by Clement Mathieu

Hi jandockx,

The default localeList is the same since 1.8 so it's not new in 1.9.3. From which version are you migrating to 1.9.3 ?

Also do you have a test-case that would demonstrate the problem ?

Thanks, Clément

comment:28 in reply to:  27 Changed 5 years ago by jandockx

We migrated from 1.9.2. No test-case available at the moment. Sorry.

Replying to cmathieu:

Hi jandockx,

The default localeList is the same since 1.8 so it's not new in 1.9.3. From which version are you migrating to 1.9.3 ?

Also do you have a test-case that would demonstrate the problem ?

Thanks, Clément

comment:29 Changed 5 years ago by Clement Mathieu

Hi Jan,

I have been able to track down the bug. I will post a test case tomorrow. Anyway, the bug is pretty subtle so it will take some time to fix it.

Regards, Clément

comment:30 Changed 5 years ago by Adrian Vasiliu

Milestone: 1.9.3
Resolution: fixed
Status: closedreopened

Reopened on cmathieu's behalf.

Changed 5 years ago by Clement Mathieu

Attachment: regressionCase2.zip added

Test case for the second regression.

comment:31 Changed 5 years ago by Clement Mathieu

I have made a test case to demonstrate the second regression.

To run, copy dojo and util directory as siblings of the app directory. Then build the app using app/dojo.profile.js and navigate to app/demo.html and open the console to see that the root bundle for number is used instead of the french one.

comment:32 Changed 5 years ago by Wulf

The issue disappeared, when I replaced the compiled Version of the ./nls/-path with the original one. The dojo-compiler changes messages.js and adds a parameter at the beginning of the define()-statement:

>>built define("module/nls/messages",{root:{...},de:!0}); # sourceMappingURL=messages.js.map

When I simply delete this parameter like

>>built define({root:{...},de:!0}); # sourceMappingURL=messages.js.map

everything works fine again.

Tested under Win 7 and dojo 1.9.3

I hope this helps to find a solution.

comment:33 Changed 5 years ago by dylan

Milestone: 1.10
Owner: changed from Rawld Gill to Colin Snover
Status: reopenedassigned

comment:34 Changed 4 years ago by bill

Milestone: 1.101.11

As per meeting.

comment:35 Changed 3 years ago by dylan

Priority: highblocker

comment:36 Changed 3 years ago by dylan

Milestone: 1.111.12

Unfortunately due to a lack of recent activity, we're going to have to punt this one until after 1.11.

comment:37 Changed 2 years ago by dylan

Milestone: 1.131.15

Ticket planning... move current 1.13 tickets out to 1.15 to make it easier to move tickets into the 1.13 milestone.

Note: See TracTickets for help on using tickets.