Skip to content
This repository

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

i18n javascript_catalog View Pollutes Global Namespace #13

Closed
wants to merge 14 commits into from

6 participants

Matthew Dapena-Tretter Florian Apolloner Alexey Boriskin Aymeric Augustin Jannis Leidel Ramiro Morales
Matthew Dapena-Tretter

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.

Florian Apolloner
Owner

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

Matthew Dapena-Tretter

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

Florian Apolloner
Owner

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

Alexey Boriskin
uruz commented

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.

Matthew Dapena-Tretter

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.

Florian Apolloner
Owner

That would definitely help, @jezdez any objections?

Matthew Dapena-Tretter

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.

Aymeric Augustin
Owner

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!

matthewwithanm and others added some commits
Matthew Dapena-Tretter matthewwithanm Use module pattern to prevent global scope pollution db2cdf1
Matthew Dapena-Tretter matthewwithanm Use object literals for dictionaries 82a13d3
Matthew Dapena-Tretter 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.
dfe7953
Matthew Dapena-Tretter matthewwithanm End function definitions with semicolons 592d774
Matthew Dapena-Tretter matthewwithanm Serialize formats dict with simplejson
Also, include all keys in object definition instead of using repeated
assignment.
861bd70
Matthew Dapena-Tretter matthewwithanm A little PEP8ing a540d58
Matthew Dapena-Tretter

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

Aymeric Augustin
Owner

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

Matthew Dapena-Tretter

No problem. There they be (:

Jannis Leidel
Owner

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

Matthew Dapena-Tretter

@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 Morales ramiro commented on the diff
django/views/i18n.py
((29 lines not shown))
77 66
78   -var catalog = new Array();
79   -"""
  67 +js_catalog_template = r"""
3
Ramiro Morales Owner
ramiro added a note

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

@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 Morales Owner
ramiro added a note

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

js_catalog_template = \
r"""{% autoescape off %}
...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
matthewwithanm added some commits
Matthew Dapena-Tretter matthewwithanm Merge branch 'master' into js-i18n-patch e5ece5f
Matthew Dapena-Tretter matthewwithanm Update test
This test was expecting the catalog to be in the old form.
ad0705c
Matthew Dapena-Tretter 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.
9ed390c
Matthew Dapena-Tretter matthewwithanm Fix context separator
There was an extra escape character since we're using a raw string for
the template.
e2e5a72
Matthew Dapena-Tretter matthewwithanm Use json instead of simplejson 2dd49b3
Matthew Dapena-Tretter matthewwithanm Clean up indentation code d954aec
Matthew Dapena-Tretter

@ramiro Let me know if anything is missing.

Ramiro Morales
Owner

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

Matthew Dapena-Tretter

No problem; thanks for reviewing!

Ramiro Morales
Owner

Fixed in a506b69. Thanks!

Ramiro Morales ramiro closed this
zmetcalf zmetcalf referenced this pull request in fusionbox/django-widgy
Closed

JS i18n - Issue #82 #140

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

Showing 14 unique commits by 2 authors.

Feb 01, 2013
Matthew Dapena-Tretter matthewwithanm Use module pattern to prevent global scope pollution db2cdf1
Matthew Dapena-Tretter matthewwithanm Use object literals for dictionaries 82a13d3
Matthew Dapena-Tretter 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.
dfe7953
Matthew Dapena-Tretter matthewwithanm End function definitions with semicolons 592d774
Matthew Dapena-Tretter matthewwithanm Serialize formats dict with simplejson
Also, include all keys in object definition instead of using repeated
assignment.
861bd70
Matthew Dapena-Tretter matthewwithanm A little PEP8ing a540d58
Feb 23, 2013
Matthew Dapena-Tretter matthewwithanm Use django namespace for i18n functions 952f32a
Matthew Dapena-Tretter matthewwithanm Extend django namespace if it exists 95b1fe3
Mar 05, 2013
Matthew Dapena-Tretter matthewwithanm Merge branch 'master' into js-i18n-patch e5ece5f
Matthew Dapena-Tretter matthewwithanm Update test
This test was expecting the catalog to be in the old form.
ad0705c
Matthew Dapena-Tretter 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.
9ed390c
Matthew Dapena-Tretter matthewwithanm Fix context separator
There was an extra escape character since we're using a raw string for
the template.
e2e5a72
Matthew Dapena-Tretter matthewwithanm Use json instead of simplejson 2dd49b3
Matthew Dapena-Tretter matthewwithanm Clean up indentation code d954aec
This page is out of date. Refresh to see the latest.
5 django/contrib/admin/static/admin/js/jquery.init.js
@@ -3,6 +3,5 @@
3 3 * namespace (i.e. this preserves pre-existing values for both window.$ and
4 4 * window.jQuery).
5 5 */
6   -var django = {
7   - "jQuery": jQuery.noConflict(true)
8   -};
  6 +var django = django || {};
  7 +django.jQuery = jQuery.noConflict(true);
240 django/views/i18n.py
... ... @@ -1,11 +1,12 @@
  1 +import json
1 2 import os
2 3 import gettext as gettext_module
3 4
4 5 from django import http
5 6 from django.conf import settings
  7 +from django.template import Context, Template
6 8 from django.utils import importlib
7 9 from django.utils.translation import check_for_language, activate, to_locale, get_language
8   -from django.utils.text import javascript_quote
9 10 from django.utils.encoding import smart_text
10 11 from django.utils.formats import get_format_modules, get_format
11 12 from django.utils._os import upath
@@ -38,6 +39,7 @@ def set_language(request):
38 39 response.set_cookie(settings.LANGUAGE_COOKIE_NAME, lang_code)
39 40 return response
40 41
  42 +
41 43 def get_formats():
42 44 """
43 45 Returns all formats strings required for i18n to work
@@ -53,120 +55,142 @@ def get_formats():
53 55 for module in [settings] + get_format_modules(reverse=True):
54 56 for attr in FORMAT_SETTINGS:
55 57 result[attr] = get_format(attr)
56   - src = []
  58 + formats = {}
57 59 for k, v in result.items():
58 60 if isinstance(v, (six.string_types, int)):
59   - src.append("formats['%s'] = '%s';\n" % (javascript_quote(k), javascript_quote(smart_text(v))))
  61 + formats[k] = smart_text(v)
60 62 elif isinstance(v, (tuple, list)):
61   - v = [javascript_quote(smart_text(value)) for value in v]
62   - src.append("formats['%s'] = ['%s'];\n" % (javascript_quote(k), "', '".join(v)))
63   - return ''.join(src)
64   -
65   -NullSource = """
66   -/* gettext identity library */
67   -
68   -function gettext(msgid) { return msgid; }
69   -function ngettext(singular, plural, count) { return (count == 1) ? singular : plural; }
70   -function gettext_noop(msgid) { return msgid; }
71   -function pgettext(context, msgid) { return msgid; }
72   -function npgettext(context, singular, plural, count) { return (count == 1) ? singular : plural; }
73   -"""
  63 + formats[k] = [smart_text(value) for value in v]
  64 + return formats
74 65
75   -LibHead = """
76   -/* gettext library */
77 66
78   -var catalog = new Array();
79   -"""
  67 +js_catalog_template = r"""
  68 +{% autoescape off %}
  69 +(function (globals) {
80 70
81   -LibFoot = """
82   -
83   -function gettext(msgid) {
84   - var value = catalog[msgid];
85   - if (typeof(value) == 'undefined') {
86   - return msgid;
87   - } else {
88   - return (typeof(value) == 'string') ? value : value[0];
89   - }
90   -}
91   -
92   -function ngettext(singular, plural, count) {
93   - value = catalog[singular];
94   - if (typeof(value) == 'undefined') {
95   - return (count == 1) ? singular : plural;
96   - } else {
97   - return value[pluralidx(count)];
98   - }
99   -}
100   -
101   -function gettext_noop(msgid) { return msgid; }
102   -
103   -function pgettext(context, msgid) {
104   - var value = gettext(context + '\\x04' + msgid);
105   - if (value.indexOf('\\x04') != -1) {
106   - value = msgid;
107   - }
108   - return value;
109   -}
110   -
111   -function npgettext(context, singular, plural, count) {
112   - var value = ngettext(context + '\\x04' + singular, context + '\\x04' + plural, count);
113   - if (value.indexOf('\\x04') != -1) {
114   - value = ngettext(singular, plural, count);
115   - }
116   - return value;
117   -}
118   -"""
  71 + var django = globals.django || (globals.django = {});
119 72
120   -LibFormatHead = """
121   -/* formatting library */
  73 + {% if plural %}
  74 + django.pluralidx = function (n) {
  75 + var v={{ plural }};
  76 + if (typeof(v) == 'boolean') {
  77 + return v ? 1 : 0;
  78 + } else {
  79 + return v;
  80 + }
  81 + };
  82 + {% else %}
  83 + django.pluralidx = function (count) { return (count == 1) ? 0 : 1; };
  84 + {% endif %}
122 85
123   -var formats = new Array();
  86 + {% if catalog_str %}
  87 + /* gettext library */
124 88
125   -"""
  89 + django.catalog = {{ catalog_str }};
126 90
127   -LibFormatFoot = """
128   -function get_format(format_type) {
129   - var value = formats[format_type];
  91 + django.gettext = function (msgid) {
  92 + var value = django.catalog[msgid];
  93 + if (typeof(value) == 'undefined') {
  94 + return msgid;
  95 + } else {
  96 + return (typeof(value) == 'string') ? value : value[0];
  97 + }
  98 + };
  99 +
  100 + django.ngettext = function (singular, plural, count) {
  101 + value = django.catalog[singular];
  102 + if (typeof(value) == 'undefined') {
  103 + return (count == 1) ? singular : plural;
  104 + } else {
  105 + return value[django.pluralidx(count)];
  106 + }
  107 + };
  108 +
  109 + django.gettext_noop = function (msgid) { return msgid; };
  110 +
  111 + django.pgettext = function (context, msgid) {
  112 + var value = django.gettext(context + '\x04' + msgid);
  113 + if (value.indexOf('\x04') != -1) {
  114 + value = msgid;
  115 + }
  116 + return value;
  117 + };
  118 +
  119 + django.npgettext = function (context, singular, plural, count) {
  120 + var value = django.ngettext(context + '\x04' + singular, context + '\x04' + plural, count);
  121 + if (value.indexOf('\x04') != -1) {
  122 + value = django.ngettext(singular, plural, count);
  123 + }
  124 + return value;
  125 + };
  126 + {% else %}
  127 + /* gettext identity library */
  128 +
  129 + django.gettext = function (msgid) { return msgid; };
  130 + django.ngettext = function (singular, plural, count) { return (count == 1) ? singular : plural; };
  131 + django.gettext_noop = function (msgid) { return msgid; };
  132 + django.pgettext = function (context, msgid) { return msgid; };
  133 + django.npgettext = function (context, singular, plural, count) { return (count == 1) ? singular : plural; };
  134 + {% endif %}
  135 +
  136 + django.interpolate = function (fmt, obj, named) {
  137 + if (named) {
  138 + return fmt.replace(/%\(\w+\)s/g, function(match){return String(obj[match.slice(2,-2)])});
  139 + } else {
  140 + return fmt.replace(/%s/g, function(match){return String(obj.shift())});
  141 + }
  142 + };
  143 +
  144 +
  145 + /* formatting library */
  146 +
  147 + django.formats = {{ formats_str }};
  148 +
  149 + django.get_format = function (format_type) {
  150 + var value = django.formats[format_type];
130 151 if (typeof(value) == 'undefined') {
131 152 return format_type;
132 153 } else {
133 154 return value;
134 155 }
135   -}
136   -"""
  156 + };
137 157
138   -SimplePlural = """
139   -function pluralidx(count) { return (count == 1) ? 0 : 1; }
140   -"""
  158 + /* add to global namespace */
  159 + globals.pluralidx = django.pluralidx;
  160 + globals.gettext = django.gettext;
  161 + globals.ngettext = django.ngettext;
  162 + globals.gettext_noop = django.gettext_noop;
  163 + globals.pgettext = django.pgettext;
  164 + globals.npgettext = django.npgettext;
  165 + globals.interpolate = django.interpolate;
  166 + globals.get_format = django.get_format;
141 167
142   -InterPolate = r"""
143   -function interpolate(fmt, obj, named) {
144   - if (named) {
145   - return fmt.replace(/%\(\w+\)s/g, function(match){return String(obj[match.slice(2,-2)])});
146   - } else {
147   - return fmt.replace(/%s/g, function(match){return String(obj.shift())});
148   - }
149   -}
  168 +}(this));
  169 +{% endautoescape %}
150 170 """
151 171
152   -PluralIdx = r"""
153   -function pluralidx(n) {
154   - var v=%s;
155   - if (typeof(v) == 'boolean') {
156   - return v ? 1 : 0;
157   - } else {
158   - return v;
159   - }
160   -}
161   -"""
  172 +
  173 +def render_javascript_catalog(catalog=None, plural=None):
  174 + template = Template(js_catalog_template)
  175 + indent = lambda s: s.replace('\n', '\n ')
  176 + context = Context({
  177 + 'catalog_str': indent(json.dumps(
  178 + catalog, sort_keys=True, indent=2)) if catalog else None,
  179 + 'formats_str': indent(json.dumps(
  180 + get_formats(), sort_keys=True, indent=2)),
  181 + 'plural': plural,
  182 + })
  183 +
  184 + return http.HttpResponse(template.render(context), 'text/javascript')
  185 +
162 186
163 187 def null_javascript_catalog(request, domain=None, packages=None):
164 188 """
165 189 Returns "identity" versions of the JavaScript i18n functions -- i.e.,
166 190 versions that don't actually do anything.
167 191 """
168   - src = [NullSource, InterPolate, LibFormatHead, get_formats(), LibFormatFoot]
169   - return http.HttpResponse(''.join(src), 'text/javascript')
  192 + return render_javascript_catalog()
  193 +
170 194
171 195 def javascript_catalog(request, domain='djangojs', packages=None):
172 196 """
@@ -243,42 +267,32 @@ def javascript_catalog(request, domain='djangojs', packages=None):
243 267 locale_t.update(catalog._catalog)
244 268 if locale_t:
245 269 t = locale_t
246   - src = [LibHead]
247 270 plural = None
248 271 if '' in t:
249 272 for l in t[''].split('\n'):
250 273 if l.startswith('Plural-Forms:'):
251   - plural = l.split(':',1)[1].strip()
  274 + plural = l.split(':', 1)[1].strip()
252 275 if plural is not None:
253 276 # this should actually be a compiled function of a typical plural-form:
254 277 # Plural-Forms: nplurals=3; plural=n%10==1 && n%100!=11 ? 0 : n%10>=2 && n%10<=4 && (n%100<10 || n%100>=20) ? 1 : 2;
255   - plural = [el.strip() for el in plural.split(';') if el.strip().startswith('plural=')][0].split('=',1)[1]
256   - src.append(PluralIdx % plural)
257   - else:
258   - src.append(SimplePlural)
259   - csrc = []
  278 + plural = [el.strip() for el in plural.split(';') if el.strip().startswith('plural=')][0].split('=', 1)[1]
  279 +
260 280 pdict = {}
  281 + maxcnts = {}
  282 + catalog = {}
261 283 for k, v in t.items():
262 284 if k == '':
263 285 continue
264 286 if isinstance(k, six.string_types):
265   - csrc.append("catalog['%s'] = '%s';\n" % (javascript_quote(k), javascript_quote(v)))
  287 + catalog[k] = v
266 288 elif isinstance(k, tuple):
267   - if k[0] not in pdict:
268   - pdict[k[0]] = k[1]
269   - else:
270   - pdict[k[0]] = max(k[1], pdict[k[0]])
271   - csrc.append("catalog['%s'][%d] = '%s';\n" % (javascript_quote(k[0]), k[1], javascript_quote(v)))
  289 + msgid = k[0]
  290 + cnt = k[1]
  291 + maxcnts[msgid] = max(cnt, maxcnts.get(msgid, 0))
  292 + pdict.setdefault(msgid, {})[cnt] = v
272 293 else:
273 294 raise TypeError(k)
274   - csrc.sort()
275 295 for k, v in pdict.items():
276   - src.append("catalog['%s'] = [%s];\n" % (javascript_quote(k), ','.join(["''"]*(v+1))))
277   - src.extend(csrc)
278   - src.append(LibFoot)
279   - src.append(InterPolate)
280   - src.append(LibFormatHead)
281   - src.append(get_formats())
282   - src.append(LibFormatFoot)
283   - src = ''.join(src)
284   - return http.HttpResponse(src, 'text/javascript')
  296 + catalog[k] = [v.get(i, '') for i in range(maxcnts[msgid] + 1)]
  297 +
  298 + return render_javascript_catalog(catalog, plural)
4 tests/view_tests/tests/i18n.py
@@ -62,12 +62,12 @@ def test_jsi18n(self):
62 62 trans_txt = catalog.ugettext('this is to be translated')
63 63 response = self.client.get('/views/jsi18n/')
64 64 # in response content must to be a line like that:
65   - # catalog['this is to be translated'] = 'same_that_trans_txt'
  65 + # "this is to be translated": "same_that_trans_txt"
66 66 # javascript_quote is used to be able to check unicode strings
67 67 self.assertContains(response, javascript_quote(trans_txt), 1)
68 68 if lang_code == 'fr':
69 69 # Message with context (msgctxt)
70   - self.assertContains(response, "['month name\x04May'] = 'mai';", 1)
  70 + self.assertContains(response, '"month name\u0004May": "mai"', 1)
71 71
72 72
73 73 class JsI18NTests(TestCase):

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.