Opened 12 years ago

Closed 12 years ago

Last modified 12 years ago

#3801 closed defect (fixed)

i18n xdomain problem

Reported by: Robert Coup Owned by: James Burke
Priority: high Milestone: 0.9
Component: Loader Version: 0.9
Keywords: Cc: James Burke, Adam Peller
Blocked By: Blocking:

Description

When i load i18n bundles after an x-domain build I get an error message after the x-d timeout expires: "Could not load cross-domain resources: null". The strings.xd.js file is found and loaded correctly by the browser.

Debugging, I found that dojo.i18n._requireLocalization() calls dojo._loadPath() passing null as the module name (which is why the error message has 'null'). In x-d, dojo._xdInFlight is a map of the loading modules, but since 'null' gets entered as the key, xdResourceLoaded doesn't clear it out of the map (since it uses 'acme.thing.nls.strings').

'acme.thing.nls.strings' is available in _requireLocalization() as bundlePackage. If we pass that instead of the null, everything works. In same-domain, the null means _loadUri() is called instead of _loadUriAndCheck() but since dojo.provide() is called just before the _loadPath() it works there as well.

Attachments (2)

3801.patch (2.0 KB) - added by Robert Coup 12 years ago.
Patch to util/ at r9720
3801.2.patch (3.3 KB) - added by Robert Coup 12 years ago.
2nd patch (replacement)

Download all attachments as: .zip

Change History (17)

comment:1 Changed 12 years ago by Robert Coup

Resolution: fixed
Status: newclosed

(In [9708]) Fixes #3801

comment:2 Changed 12 years ago by Robert Coup

Went through the fix with peller on irc - jburke, if you can spot any issues let me know.

comment:3 Changed 12 years ago by James Burke

Resolution: fixed
Status: closedreopened

rcoup, were you seeing this issue after r9573? I put in a fix for a similar issue for trac #3702 in r9573.

Normally, I try to eval local i18n modules immediately, and it is what r9573 was trying to fix. That was the behavior in 0.4.3, and I forgot to fully implement it in 0.9 (I was trying to be slick and dynamically load dojo.i18n when it was needed, but it got pretty complicated, and I forgot to reimplement the 0.4.3 approach).

Or, were you seeing an issue with an i18n bundle that you made that was turned into an xdomain bundle? For instance, acme/thing/nls/strings.js, you do an xdomain build so you can load this xdomain as acme/thing/nls/strings.xd.js?

If that is the case, I would expect other dojo code to fail, in particularly things like dijit calendar. But maybe those were complicated enough loading that things worked out?

Hmm, in an xdomain build, an xdomain module doesn't even call dojo._requireLocalization, the calls are converted to dojo.xdRequireLocalization(), which do a plain dojo.require() underneath.

It would be good to verify that this problem is seen even after r9573. Something seems weird.

Reopening the ticket, until we can confirm the scenario a bit more.

comment:4 Changed 12 years ago by James Burke

Just to clarify r9573 was done after the 0.9.0 beta release.

comment:5 Changed 12 years ago by Robert Coup

Yeah, I've been working off HEAD and I only just started looking into xd-builds over the last day or two. So all after r9573.

Original code built into dojo.xd.js (ported from 0.4.3ish):

dojo.requireLocalization("otm.map","strings");
acme.thing.strings = {};
dojo.addOnLoad(function() {
    // this needs to be here due to an issue with xd-loading (rc/jburk@dojo 03/05/07)
    acme.thing.strings = dojo.i18n.getLocalization("acme.thing","strings");
});

I get the same results (ie. works with patch, doesn't work without) from:

dojo.requireLocalization("acme.thing","strings");
acme.thing.strings = dojo.i18n.getLocalization("acme.thing","strings");

At xd-build time the requireLocalization() call gets converted into:

dojo.requireLocalization("acme.thing","strings", null, "de,ROOT");

Or, were you seeing an issue with an i18n bundle that you made that was turned into an xdomain bundle? For instance, acme/thing/nls/strings.js, you do an xdomain build so you can load this xdomain as acme/thing/nls/strings.xd.js?

Yes

If that is the case, I would expect other dojo code to fail, in particularly things like dijit calendar. But maybe those were complicated enough loading that things worked out?

I'll have a closer look at the calendar.

Hmm, in an xdomain build, an xdomain module doesn't even call dojo._requireLocalization, the calls are converted to dojo.xdRequireLocalization(), which do a plain dojo.require() underneath.

Where? The built .xd.js file has requireLocalization(), _xdCreateResource() doesn't replace it with a require(), and it doesn't seem to remap dojo.requireLocalization anywhere either.

comment:6 Changed 12 years ago by James Burke

I think I can see a problem if the code is in a layered file, like dojo.xd.js, but I'm still not following something. You mention that the following works with the patch but not without:

dojo.requireLocalization("acme.thing","strings");
acme.thing.strings = dojo.i18n.getLocalization("acme.thing","strings");

So this code works in a built dojo.xd.js (or a layer file) without having to wrap the dojo.i18n.getLocalization call in a dojo.addOnLoad call? I would expect it not to work if the acme.thing.nls.strings i18n bundle is loaded xdomain since it is an asynchronous load.

My first thought is that the original issue may be fixed by running the layered build files through the code that converts dojo.requireLocalization() calls to dojo.xdRequireLocalization() calls (done in i18nUtil.modifyRequireLocalization()) (maybe as part of the fix #2713 -- something else complicating the issue).

The xdRequireLocalization call remembers what locale you asked for, but only loads a locale that is available (using the locale list listed in the 4th parameter to the requireLocalization/xdRequireLocalization call), then binds the loaded available locale to the original locale asked for.

So I would expect that even with the r9708 fix, there would be an issue if you asked for a locale that you did not have as an actual bundle. But maybe that is not the case?

comment:7 Changed 12 years ago by Robert Coup

Ok, more investigation:

You mention that the following works with the patch but not without

was incorrect. I think there was a timing issue in our code which meant it worked sometimes and re-called getLocalization() sometimes. certainly if you do acme.thing.strings.test_string directly after the getLocalization() call it's empty.

My first thought is that the original issue may be fixed by running the layered build files through the code that converts dojo.requireLocalization() calls to dojo.xdRequireLocalization()

that works, with and without r9708.

So I would expect that even with the r9708 fix, there would be an issue if you asked for a locale that you did not have as an actual bundle. But maybe that is not the case?

Yep, thats true. If you call requireLocalization() for a locale you don't have it chucks exceptions. Calling getLocalization() is ok (it just returns ROOT with no requests).

comment:8 Changed 12 years ago by Robert Coup

After discussion with jburke, this patch changes the build process to replace requireLocalization() calls with 'xdRequireLocalization()` for combined layer files (eg. dojo.xd.js).

This makes r9708 redundant so it should be reverted.

See #3816 for related stuff - after that is fixed, requireLocalization() could be changed to a check for x-domain resources and a branch between xdRequireLocalization() and the existing requireLocalization()

Changed 12 years ago by Robert Coup

Attachment: 3801.patch added

Patch to util/ at r9720

comment:9 Changed 12 years ago by Robert Coup

More discussion and a new patch. This time we replace requireLocalization with a function that determines whether to call xdRequireLocalization() or the original requireLocalization(). In this way, users and the build system don't have to worry about which one is appropriate for each module.

Changed 12 years ago by Robert Coup

Attachment: 3801.2.patch added

2nd patch (replacement)

comment:10 Changed 12 years ago by Robert Coup

(In [9724]) Revert [9708] and make dojo.requireLocalization smarter about local vs xd when we're in a cross-domain scenario. Break out check for xd or local from _loadPath to support it. Refs #3801

comment:11 Changed 12 years ago by Robert Coup

Not closing yet, jburke wants to do some build-time cleanups around bundles & xd.

comment:12 Changed 12 years ago by James Burke

Some small cleanup to get rid of the direct dojo.xdRequireLocalization calls in the build:

util/buildscripts/jslib/buildUtilXd.js: Keep the work it does in makeXdContents, but change this line to use "requireLocalization":

depCall = "xdRequireLocalization";

util/buildscripts/jslib/makeDojoJsWeb.js: Remove the regexp that is done for xdRequireLocalization (don't need to test this file since it hasn't been updated to 0.9 though).

comment:13 Changed 12 years ago by James Burke

Owner: changed from Robert Coup to James Burke
Status: reopenednew

rcoup, i'm taking this bug just to clean up the last two build things mentioned in the last comment, since I'm in there right now anyway.

comment:14 Changed 12 years ago by James Burke

Resolution: fixed
Status: newclosed

(In [10076]) Fixes #2713 #4085 #3801. Flattened bundles for i18n layers should work in xd and non-xd load cases now. Also brought over some xd fixes that were made after the 0.9 code was copied from the old trunk.

comment:15 Changed 12 years ago by James Burke

Resolution: fixed

(In [10077]) Fixes #2713 #4085 #3801. Flattened bundles for i18n layers should work in xd and non-xd load cases now. Also brought over some xd fixes that were made after the 0.9 code was copied from the old trunk.

Note: See TracTickets for help on using tickets.