Internationalize units and currency #780

JonathanDCohen · 2020-12-17T07:15:03Z

This required updating to Typescript 4 and adding compiler changes for the server manager in order to support newer features of Intl.

This change also fixes it so that the server city, creation date, cost, server data cap, data limits, etc are no longer stuck to the old language after changing languages. Some of these fixes only work the first time you switch languages, but not if you switch again. I have no idea why that is, but quitting and restarting the app still gets everything back in sync.

This change un-broke the web app tests by correctly building the digitalocean install script to avoid import errors.

This required updating to Typescript 4 and adding compiler changes for the server manager in order to support newer features of `Intl`. This change also fixes it so that the server city, creation date, and data limits no longer are stuck to the old language ater changing languages.

fortuna

Thanks for cleaning up the units.

src/server_manager/web_app/ui_components/outline-server-view.js

src/server_manager/web_app/i18n_formatting.ts

src/server_manager/web_app/app.ts

src/server_manager/tsconfig.json

src/server_manager/web_app/app.ts

package.json

src/server_manager/tsconfig.json

package.json

src/server_manager/tsconfig.json

src/server_manager/web_app/app.ts

src/server_manager/infrastructure/data_formatting.ts

src/server_manager/infrastructure/data_formatting.spec.ts

src/server_manager/web_app/karma.conf.js

src/server_manager/infrastructure/data_formatting.ts

src/server_manager/web_app/ui_components/outline-server-view.js

This code is only intended to be run in a web environment, not in Node.

mpmcroy

The internationalization looks good!

And thanks for fixing the localization and karma tests 🎉

src/server_manager/web_app/data_formatting.ts

mpmcroy · 2021-01-21T14:53:03Z

src/server_manager/web_app/data_formatting.ts

+  return out;
+}
+
+export function getFormattedDataAmountParts(amount: number, language: string) {


Could you add return types and JS docs for the exported methods?

As a user, I'd be interested to know which one to choose based on my use case.

Done in 49ce60d, let me know if they're useful

mpmcroy · 2021-01-21T14:53:09Z

src/server_manager/web_app/data_formatting.ts

+  };
+}
+
+export function formatBytes(numBytes: number, language: string) {


Should we name these two methods something similar to show that they're related?

Maybe like formatBytes and formatBytesToParts?

Done in 49ce60d, that's a good suggestion

mpmcroy · 2021-01-21T15:02:55Z

src/server_manager/web_app/ui_components/outline-server-settings.js

+  },
+
+  _getInternationalizedUnit(bytesAmount, language) {
+    // This happens during app startup before we set the language


I've run into these kind of issues a few times too.

My approach has been to try to set a sensible default and always maintain an invariant that the value is not null. That allowed me to remove the null|undefined checks.

What are your thoughts?

I was unsure about which defaults to pick. I went with English and the Unix epoch, how do you think about that?

That seems like a good default to me. I suspect the user will not even see the default value since it's not the default tab.

mpmcroy · 2021-01-21T15:05:05Z

src/server_manager/web_app/ui_components/outline-server-view.js

@@ -596,7 +588,7 @@ export class ServerView extends DirMixin(PolymerElement) {
        serverManagementApiUrl: String,
        serverPortForNewAccessKeys: Number,
        isAccessKeyPortEditable: {type: Boolean},
-        serverCreationDate: String,
+        serverCreationDate: {type: Object, value: null},  // type: Date


Can you add a JS Doc type comment to the constructor instead?

Done in 49ce60d

mpmcroy · 2021-01-21T15:10:10Z

src/server_manager/web_app/data_formatting.spec.ts

+
+  describe('formatBytes', () => {
+    it('Formats data amounts', () => {
+      expect(i18n.formatBytes(10 * 10 ** 9, 'en')).toEqual('10 GB');


Can you add a KB test so that we cover that code path?

Done in 49ce60d

I don't see it there.

It's in https://github.com/Jigsaw-Code/outline-server/pull/780/commits/49ce60d8d96a8d1d434992c60e01f04c2a37af65#diff-858c37f4a836374e98fb54a760e5a4099b33a6ae04b41c2079b60c588eeb492cR31, I'm not sure why the view isn't showing it

It seems the KB test was added to the formatBytesParts above.

src/server_manager/web_app/ui_components/app-root.js

mpmcroy · 2021-01-21T15:48:51Z

src/server_manager/web_app/app.ts

+      await this.appRoot.setLanguage(languageCode, languageDir);
+      document.documentElement.setAttribute('dir', languageDir);
+      window.localStorage.setItem('overrideLanguage', languageCode);
+      this.appRoot.showServerView();


I don't think we should call showServerView here. It might be confusing to the user if the app changes screens.

I thought I had got rid of that! Done in 49ce60d

mpmcroy

I didn't take a look at the Typescript 4/ES 2020 part since I think @fortuna is better suited to review that, but the rest looks 🔥 🔥 🔥

LGTM!

src/server_manager/web_app/ui_components/app-root.js

mpmcroy · 2021-01-21T18:45:53Z

src/server_manager/web_app/ui_components/outline-server-settings.js

+
+  _getTranslatedDate(language, date) {
+    // We can't use a default Date object -- it still shows up as null, I'm not sure the lifetime
+    // properties when you pass an object into Polymer.


I wonder if this happens because we set the default value in the Polymer property configuration. I think this is one of the only files that sets defaults that way.

Does it work if you set serverCreationDate in the constructor?

I would guess that it does, but I don't want to change this file to use a constructor in this PR, it's too much going on already and it's not important enough to keep delaying the per key data limits ui for

mpmcroy · 2021-01-21T18:48:32Z

src/server_manager/web_app/ui_components/outline-server-settings.js

+  },
+
+  _getInternationalizedUnit(bytesAmount, language) {
+    // This happens during app startup before we set the language


That seems like a good default to me. I suspect the user will not even see the default value since it's not the default tab.

src/server_manager/web_app/ui_components/outline-server-view.js

src/server_manager/web_app/app.ts

fortuna · 2021-01-21T23:55:05Z

src/server_manager/web_app/data_formatting.spec.ts

+
+  describe('formatBytes', () => {
+    it('Formats data amounts', () => {
+      expect(i18n.formatBytes(10 * 10 ** 9, 'en')).toEqual('10 GB');


I don't see it there.

src/server_manager/web_app/data_formatting.ts

src/server_manager/web_app/karma.conf.js

src/server_manager/web_app/ui_components/outline-server-view.js

src/server_manager/web_app/ui_components/app-root.js

src/server_manager/tsconfig.json

fortuna

Thanks for making this change. Our unit formatting is a lot more robust now, thanks to you.

Please make sure the test is passing before submitting. It seems to be broken.

fortuna · 2021-01-27T20:17:19Z

src/server_manager/web_app/data_formatting.spec.ts

+  if (process?.versions?.node) {
+    it('doesn\'t run on Node', () => {
+      expect(() => i18n.formatBytesParts(0, 'en')).toThrow();
+    });
+  } else {
+    it('extracts the unit string and value separately', () => {
+      const english = i18n.formatBytesParts(0, 'en');
+      expect(english.unit).toEqual('B');
+      expect(english.value).toEqual('0');
+
+      const spanish = i18n.formatBytesParts(2, 'es');
+      expect(spanish.unit).toEqual('B');
+      expect(spanish.value).toEqual('2');
+
+      const russian = i18n.formatBytesParts(3000, 'ru');
+      expect(russian.unit).toEqual('кБ');
+      expect(russian.value).toEqual('3');
+
+      const french = i18n.formatBytesParts(1.5 * 10 ** 9, 'fr');
+      expect(french.unit).toEqual('Go');
+      expect(french.value).toEqual('1,5');
+
+      const farsi = i18n.formatBytesParts(133.5 * 10 ** 6, 'fa');
+      expect(farsi.unit).toEqual('مگابایت');
+      expect(farsi.value).toEqual('۱۳۳٫۵');
+    });
+  }
+});


This looks good. An alternative is to mark the test explicitly as skipped on Node, so it shows on the output:

Suggested change

if (process?.versions?.node) {

it('doesn\'t run on Node', () => {

expect(() => i18n.formatBytesParts(0, 'en')).toThrow();

});

} else {

it('extracts the unit string and value separately', () => {

const english = i18n.formatBytesParts(0, 'en');

expect(english.unit).toEqual('B');

expect(english.value).toEqual('0');

const spanish = i18n.formatBytesParts(2, 'es');

expect(spanish.unit).toEqual('B');

expect(spanish.value).toEqual('2');

const russian = i18n.formatBytesParts(3000, 'ru');

expect(russian.unit).toEqual('кБ');

expect(russian.value).toEqual('3');

const french = i18n.formatBytesParts(1.5 * 10 ** 9, 'fr');

expect(french.unit).toEqual('Go');

expect(french.value).toEqual('1,5');

const farsi = i18n.formatBytesParts(133.5 * 10 ** 6, 'fa');

expect(farsi.unit).toEqual('مگابایت');

expect(farsi.value).toEqual('۱۳۳٫۵');

});

}

});

let itBrowser = it;

if (process?.versions?.node) {

itBrowser = xit;

}

itBrowser('extracts the unit string and value separately', () => {

const english = i18n.formatBytesParts(0, 'en');

expect(english.unit).toEqual('B');

expect(english.value).toEqual('0');

const spanish = i18n.formatBytesParts(2, 'es');

expect(spanish.unit).toEqual('B');

expect(spanish.value).toEqual('2');

const russian = i18n.formatBytesParts(3000, 'ru');

expect(russian.unit).toEqual('кБ');

expect(russian.value).toEqual('3');

const french = i18n.formatBytesParts(1.5 * 10 ** 9, 'fr');

expect(french.unit).toEqual('Go');

expect(french.value).toEqual('1,5');

const farsi = i18n.formatBytesParts(133.5 * 10 ** 6, 'fa');

expect(farsi.unit).toEqual('مگابایت');

expect(farsi.value).toEqual('۱۳۳٫۵');

});

});

Up to you

That's a cool idea, but I'm going to keep it the way I wrote it because it tests the runtime check which keeps us from putting the code in Node

fortuna · 2021-01-27T20:19:10Z

src/server_manager/web_app/data_formatting.spec.ts

+
+  describe('formatBytes', () => {
+    it('Formats data amounts', () => {
+      expect(i18n.formatBytes(10 * 10 ** 9, 'en')).toEqual('10 GB');


It seems the KB test was added to the formatBytesParts above.

…ues with undefined language at startup

mpmcroy

🔥

Jonathan Cohen added 2 commits December 16, 2020 21:23

Internationalize data transfer amounts

3e4257c

JonathanDCohen requested a review from fortuna December 17, 2020 07:15

google-cla bot added the cla: yes label Dec 17, 2020

JonathanDCohen requested a review from mpmcroy December 17, 2020 07:15

Remove stray log

01fc89a

fortuna requested changes Dec 17, 2020

View reviewed changes

Jonathan Cohen added 2 commits December 17, 2020 20:24

Respond to review comments

79fb1ea

Remove byte-size

ee59df0

JonathanDCohen requested a review from fortuna December 18, 2020 01:32

fortuna requested changes Dec 21, 2020

View reviewed changes

src/server_manager/tsconfig.json Outdated Show resolved Hide resolved

fortuna requested changes Dec 23, 2020

View reviewed changes

Jonathan Cohen added 4 commits January 13, 2021 18:30

Move to web_app

a0dc712

This code is only intended to be run in a web environment, not in Node.

Merge branch 'master' into cohenjon-intl-units

c18d6f2

Respond to review comments

8c1a131

Use property binding to the app language

017bdf7

JonathanDCohen requested a review from fortuna January 20, 2021 21:44

mpmcroy reviewed Jan 21, 2021

View reviewed changes

Respond to review comments

49ce60d

JonathanDCohen requested a review from mpmcroy January 21, 2021 18:02

mpmcroy approved these changes Jan 21, 2021

View reviewed changes

fortuna requested changes Jan 22, 2021

View reviewed changes

Jonathan Cohen added 2 commits January 22, 2021 18:07

Respond to review comments

9d11943

s/get/format

435c5a5

JonathanDCohen requested review from fortuna and mpmcroy January 25, 2021 15:39

Error out on Node

6f4a323

fortuna approved these changes Jan 27, 2021

View reviewed changes

Jonathan Cohen added 2 commits February 1, 2021 20:10

Set the persisted app language before starting the app to prevent iss…

c970ee5

…ues with undefined language at startup

Merge branch 'master' into cohenjon-intl-units

2af1024

Respond to review comments and broaden covered languages

5bcc243

mpmcroy approved these changes Feb 4, 2021

View reviewed changes

JonathanDCohen merged commit 1c5b563 into master Feb 4, 2021

JonathanDCohen deleted the cohenjon-intl-units branch February 4, 2021 23:47

Internationalize units and currency #780

Internationalize units and currency #780

Uh oh!

Conversation

JonathanDCohen commented Dec 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fortuna left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mpmcroy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JonathanDCohen Jan 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mpmcroy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JonathanDCohen commented Dec 17, 2020 •

edited

Loading

JonathanDCohen Jan 22, 2021 •

edited

Loading