Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

i18n javascript_catalog View Pollutes Global Namespace #13

wants to merge 14 commits into


None yet
6 participants

matthewwithanm commented Apr 28, 2012

This pull request corresponds to Trac Ticket number 18231

In addition to the translation functions (gettext, ngettext, etc.), the global variables catalog and formats are created. These variables are required to exist and be in the correct format in order for the translation functions to work.

Seeing as how these are common words, with a relatively high likelihood of being clobbered, I propose we use the module pattern (an immediately invoked function).

This patch also uses vanilla objects for catalog and formats as those are the preferred type for dictionaries in JavaScript.


apollo13 commented Aug 13, 2012

@matthewwithanm Does this have any backward compat issues? Eg if someone used this global scope vars?


matthewwithanm commented Aug 13, 2012

Yeah, this creates the catalog and formats dictionaries in IIFEs, so they aren't global anymore.


apollo13 commented Aug 13, 2012

Hmm I guess we could provide a deprecation path for that, but who reads js warnings?!


uruz commented Aug 19, 2012

What do you think about the following idea: make all the generated javascript to be a string with django template syntax? That would allow us not to worry about indentation changes.


matthewwithanm commented Aug 19, 2012

I'm very in favor of that. I think it would also make the JS much more maintainable. I would've done that with the initial PR but thought it might be too drastic a change.


apollo13 commented Aug 20, 2012

That would definitely help, @jezdez any objections?


matthewwithanm commented Aug 20, 2012

I'm getting ready to embark on a weeklong trip, but if there aren't any objections in the meantime, I'll knock this out afterwards, before DjangoCon. I'll also update the dict creation to use object literals instead of repeated assignment.


aaugustin commented Feb 1, 2013

This PR is stale; since our triage options on GitHub are limited to "open" or "closed" I'm going to close it.

Please re-open if you have a chance to update it.

If no one reviews it, write to the django-developers mailing list. Thanks!

@aaugustin aaugustin closed this Feb 1, 2013

matthewwithanm added some commits Apr 26, 2012

@matthewwithanm @matthewwithanm matthewwithanm Use module pattern to prevent global scope pollution db2cdf1
@matthewwithanm @matthewwithanm matthewwithanm Use object literals for dictionaries 82a13d3
@matthewwithanm matthewwithanm Use template for javascript catalog
The same template is used for the `javascript_catalog` and
`null_javascript_catalog` views; they're simply supplied with different
context variables. In addition, the catalog dictionary is defined using
an object literal (serialized using simplejson), which eliminates a few
bytes when compared to the original method.
@matthewwithanm matthewwithanm End function definitions with semicolons 592d774
@matthewwithanm matthewwithanm Serialize formats dict with simplejson
Also, include all keys in object definition instead of using repeated
@matthewwithanm matthewwithanm A little PEP8ing a540d58

matthewwithanm commented Feb 2, 2013

@aaugustin I've rebased these commits onto the current master, however, I don't have permissions to re-open this issue and, unless the issue is open, I can't update the pull request. I believe if you reopen the issue I can force push to this branch and GitHub will update the PR. Alternatively, I can open a new PR if you'd rather.

@aaugustin aaugustin reopened this Feb 2, 2013


aaugustin commented Feb 2, 2013

I thought you could re-open a PR simply by pushing new commits :/


matthewwithanm commented Feb 2, 2013

No problem. There they be (:


jezdez commented Feb 23, 2013

I think using the django namespace like we do in the admin makes sense for this.


matthewwithanm commented Feb 23, 2013

@jezdez Sounds good. 952f32a makes that change and 95b1fe3 updates the other creation of the django namespace so as not to clobber it if it already exists. (I think that's the only place it's created?)

Note that I'm not exporting the catalog or format dictionaries to the global namespace but we could if we wanted to preserve backwards compatibility. (Since the functions are using the namespaced versions, they would still continue to work if somebody overwrote the global reference.)

@ramiro ramiro commented on the diff Feb 24, 2013

-var catalog = new Array();
+js_catalog_template = r"""

ramiro Feb 24, 2013


Nitpick: Append a \ to the end so it doesn't generate an empty first line.


matthewwithanm Feb 25, 2013


@ramiro I'm using a raw string, so I don't think that'll work here. We could move it onto the same line as the triple quote, but IMO the added readability in the source is worth the extra empty first line. Thoughts?


ramiro Mar 5, 2013


You are completely right. What about this? Too ugly?:

js_catalog_template = \
r"""{% autoescape off %}

matthewwithanm added some commits Mar 5, 2013

@matthewwithanm matthewwithanm Merge branch 'master' into js-i18n-patch e5ece5f
@matthewwithanm matthewwithanm Update test
This test was expecting the catalog to be in the old form.
@matthewwithanm matthewwithanm Fix bug with storage of plurals
This should also eliminate a bit of the redundancy from the original
implementation since we're constructing one array per message id,
instead of an array filled with empty string which we later fill.
@matthewwithanm matthewwithanm Fix context separator
There was an extra escape character since we're using a raw string for
the template.
@matthewwithanm matthewwithanm Use json instead of simplejson 2dd49b3
@matthewwithanm matthewwithanm Clean up indentation code d954aec

matthewwithanm commented Mar 22, 2013

@ramiro Let me know if anything is missing.


ramiro commented Mar 26, 2013

@matthewwithanm I've reviewed the PR and it's in good shape.Will commit RSN after some i18n problems in master are settled so I can test the changes in isolation and ask for opinions from a couple of devs experienced on i18n. Thanks for a great job.


matthewwithanm commented Mar 26, 2013

No problem; thanks for reviewing!


ramiro commented Apr 14, 2013

Fixed in a506b69. Thanks!

@ramiro ramiro closed this Apr 14, 2013

@zmetcalf zmetcalf referenced this pull request in fusionbox/django-widgy Nov 14, 2013


JS i18n - Issue #82 #140

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment