Opened 3 years ago

Closed 2 years ago

#18231 closed Bug (fixed)

i18n javascript_catalog View Pollutes Global Namespace

Reported by: Matthew Tretter <matthew@…> Owned by: nobody
Component: Internationalization Version: master
Severity: Normal Keywords: javascript i18n
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

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.

Attachments (1)

js-i18n-patch.diff (4.7 KB) - added by Matthew Tretter <matthew@…> 3 years ago.

Download all attachments as: .zip

Change History (10)

Changed 3 years ago by Matthew Tretter <matthew@…>

comment:1 Changed 3 years ago by Matthew Tretter <matthew@…>

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

The patch corresponds to pull request #13 on the github repository.

https://github.com/django/django/pull/13

comment:2 Changed 3 years ago by jezdez

  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Uncategorized to Bug

Accepting in general, but not generally excited about the indentation changes.

comment:3 Changed 3 years ago by void

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.

comment:4 Changed 3 years ago by matthewwithanm

New commits have been added to the PR on GitHub to use a template.

comment:5 Changed 3 years ago by aaugustin

  • Patch needs improvement set

The PR no longer applies.

comment:6 Changed 3 years ago by matthewwithanm

I've got an updated branch, but I can't update the pull request since the issue is closed. Let me know if want to reopen the PR and have me update it or if I should create a new PR instead.

comment:7 Changed 2 years ago by ramiro

Current PR is almost there but is causing one failure and seven errors in the Django test suite, all of the related to the js i18n catalog.

comment:8 Changed 2 years ago by matthewwithanm

Woops, sorry about that! I've updated the pull request.

comment:9 Changed 2 years ago by Ramiro Morales <cramm0@…>

  • Resolution set to fixed
  • Status changed from new to closed

In a506b6981bc48caec30bca3de94d2ac3e6fc1660:

Fixed #18231 -- Made JavaScript i18n not pollute global JS namespace.

Also, use Django templating for the dynamic generated JS code and use
more idiomatic coding techniques.

Thanks Matthew Tretter for the report and the patch.

Note: See TracTickets for help on using tickets.
Back to Top