Use the new localization syntax for all ES6 JavaScript.

Review Request #11001 — Created April 10, 2020 and submitted

Information

Djblets
release-1.0.x
c5f0f6c...

Reviewers

This makes use of the new babel-plugin-django-gettext template literal
support in our ES6 JavaScript files, giving us much more readable code,
especially when long strings are concerned.

We can now use the new _ prefix and break strings into multiple lines,
and the plugin will take care to clean up the whitespace and turn them
back into single-line gettext() calls. We can also reference variables
in template literals, and the plugin will turn them into interpolate()
calls (though we don't make use of that currently int his codebase).

There are still files that aren't converted to ES6 in this codebase, and
there are likely a lot of unlocalized strings, but this change doesn't
tackle either. It's really the changes that were being tested during the
development of the plugin.

Built a package and compared the generated code to the source code.
Saw that each and every new-style localization was converted to an
old-fashioned gettext(), with the expected strings.

Description From Last Updated

Can you just quickly double-check that the generated code is correct here?

daviddavid
chipx86
david
  1. This is amazing, but the dedent one still worries me. Mind doing one last check before landing?

  2. Show all issues

    Can you just quickly double-check that the generated code is correct here?

    1. It generates the code I expect, but the approach was wrong, because if a translated string contains HTML-unsafe characters, things will break. Moving back to <%- ... %>.

    2. Okay, so let's talk inline localization and escaping. I've been going back and forth on this for a couple days (wrote up stuff in Notion and.. there's just trade-offs).

      So, we have the following two approaches we can take:

      // Using <%- ... %> and wrapping in quotes
      const s = _.template(dedent`
          <p>
           <%- "${_`This is some text to localize, which requires escaping \\" or ', depending on the wrapping quotes.`}" %>
          </p>
      `);
      
      // Not using `<%` of any form, but manually calling _.escape:
      const s = _.template(dedent`
          <p>
           ${_.escape(_`This is some text to localize, which freely allows " or '.`)}
          </p>
      `);
      

      This is (very roughly) equivalent to the following:

      const s = _.template('<p><%- "' + gettext(...) + '" %></p>');
      
      const s = _.template('<p>' + _.escape(gettext(...)) + '</p>');
      

      You can see how the former, while less verbose, requires us to consider what sort of quoting we have in the message, and this could pose issues. This is because <%- requires a value (string in our case), and we can't just append a string (the gettext call) to it, since that'll otherwise look something like <%- This is some text to localize.... %>, so we have to quote.

      The latter is more verbose, a bit uglier, but avoids the issue by building an escaped string and concatening it, rather than passing it in to a <% tag.

      I'm leaning toward us using the latter for now, since it's safer and less constricting. We could add some syntactic sugar through another plugin at some point maybe... I don't know.

      What do you think?

  3. 
      
chipx86
david
  1. Ship It!
  2. 
      
chipx86
Review request changed
Status:
Completed
Change Summary:
Pushed to master (896ad0b)