Typescript #1106

kturney · 2020-01-16T20:25:52Z

This PR converts the js in the addon folder to typescript.

Sorry for submitting this PR without discussion first.
It started out as an "I wonder if I can..." after reading #733 (comment).
I'm also submitting it in the spirit of typed-ember/ember-cli-typescript#48.

If we like this, I can also set about converting the addon-test-support code.

kturney · 2020-01-16T20:32:47Z

It seems tests are failing because travis is using node 8 and a dep wants node 10.
I can update the node version for travis in this PR, if desired.

jasonmit · 2020-01-16T21:56:39Z

It seems tests are failing because travis is using node 8 and a dep wants node 10.

I'd like to drop node 8 as part of 5.0.0-stable so feel free to bump in the Travis config and make the commit with BREAKING.

As for the rest of the PR, I'll need some time to go over it. Thanks a ton for taking this on!

jasonmit · 2020-01-17T10:36:24Z

addon/models/translation.ts

+export class TranslationModel extends EmberObject {
+  translations: Map<string, string> = new Map();
+
+  init() {


should this be done in the constructor?

It's my understanding that the declaration (translations: Map<string, string> = new Map();) should have initialized translations in the constructor. For some reason in older ember versions that was not the case, so I initialize in init if it's missing. I can investigate wether manually initializing in the constructor instead of init works in the older versions.

The ember-lts-2.16 and ember-lts-2.18 scenarios fail when initializing in the constructor instead of init.

jasonmit · 2020-01-17T10:37:41Z

addon/models/translation.ts

+  /**
+   * Adds a translation hash
+   */
+  addTranslations(translations: any) {


I think should be Map<string, string>

this translations param is a JSON blob, I believe, but maybe something like the following would suffice?

interface Translations { [key: string]: Translations | string }

That works!

If it's a JSON blob, you probably want it to be [key: string]: Translations | string | undefined.

Given how common this particular kind of thing is, you may want to define a Dict type like this:

interface Dict<T> { [key: string]: T | undefined; }

Then in scenarios like this you'd type it as:

export class TranslationModel extends EmberObject { // ... addTranslations(translations: Dict<Translations | string>) {

jasonmit · 2020-01-17T10:38:39Z

addon/services/intl.ts


+    set(this: OverridableProps & IntlService, _, localeName: LocaleName) {
+      // console.log('set locale', localeName);


jasonmit · 2020-01-17T10:39:06Z

addon/services/intl.ts

      if (!isArrayEqual(proposed, this._locale)) {
        this._locale = proposed;
-        cancel(this._timer);
-        this._timer = next(() => this.trigger('localeChanged'));
+        if (this._timer) cancel(this._timer);


nit:

if (this._timer) { cancel(this._timer); }

jasonmit · 2020-01-17T10:39:52Z

addon/services/intl.ts


  /** @public **/
-  addTranslations(localeName, payload) {
+  addTranslations(localeName: string, payload: any) {


Map<string, string>

jasonmit · 2020-01-17T21:11:34Z

addon/-private/formatters/-base.ts

-export default class FormatterBase {
-  get options() {
-    return emberArray();
+export interface FormatterOptions {


Could be typed per each formatter since they have slightly different shapes/options

I like it. Will do.

jasonmit · 2020-01-17T21:12:03Z

addon/-private/formatters/-base.ts

+  format(value: any, options?: object): FormatResult;
+}
+
+export default interface Formatter<T> {


If you fix the above, this would become Formatter<T, FormatterOptionsType>

jasonmit · 2020-01-17T21:13:24Z

addon/-private/normalize-locale.ts

@@ -0,0 +1,15 @@
+import { assert } from '@ember/debug';
+
+function assertIsString(val: any): asserts val is string {


asserts val is string - Is this a typescript feature? If so, pretty awesome!

a brand new feature! https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-7.html#assertion-functions

jasonmit · 2020-01-17T21:14:08Z

Took a first pass at it, looks great so far! Left some comments.

kturney

Thanks for the quick feedback!
I'll get these fixed up tomorrow.

kturney · 2020-01-18T05:12:23Z

addon/models/translation.ts

+  /**
+   * Adds a translation hash
+   */
+  addTranslations(translations: any) {


this translations param is a JSON blob, I believe, but maybe something like the following would suffice?

interface Translations { [key: string]: Translations | string }

kturney · 2020-01-18T05:15:32Z

addon/models/translation.ts

+export class TranslationModel extends EmberObject {
+  translations: Map<string, string> = new Map();
+
+  init() {


It's my understanding that the declaration (translations: Map<string, string> = new Map();) should have initialized translations in the constructor. For some reason in older ember versions that was not the case, so I initialize in init if it's missing. I can investigate wether manually initializing in the constructor instead of init works in the older versions.

kturney · 2020-01-18T05:20:04Z

addon/-private/formatters/-base.ts

-export default class FormatterBase {
-  get options() {
-    return emberArray();
+export interface FormatterOptions {


I like it. Will do.

kturney · 2020-01-18T05:23:28Z

addon/-private/normalize-locale.ts

@@ -0,0 +1,15 @@
+import { assert } from '@ember/debug';
+
+function assertIsString(val: any): asserts val is string {


a brand new feature! https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-7.html#assertion-functions

kturney · 2020-01-29T17:51:50Z

I rebased to get travis running again.
https://travis-ci.community/t/travis-ci-stopped-tracking-building-a-pr/4159/3

jasonmit · 2020-01-31T20:47:13Z

CI should now be fixed if you rebase

fabmiz · 2020-02-21T14:46:39Z

@kturney; @buschtoens are there reasons this is not getting your thumbs up? I could help if need be.

buschtoens · 2020-02-21T16:47:51Z

Time only, I guess. 😅

I've moved this to my weekend to-do list now. I hope I get some time for it. Thank you for pinging!

buschtoens

Looking great already! Thanks for this awesome PR. 🎉

I only got to review a first chunk so far and will try to follow-up with the rest ASAP.

buschtoens · 2020-03-01T10:00:39Z

.eslintrc.js

+    // all TypeScript files
+    {
+      files: ['**/*.ts'],
+      rules: {
+        // These are covered by tsc
+        'no-undef': 'off',
+        'no-unused-vars': 'off'
+      }


This is no critique against this PR in any way and more of a note to myself and other maintainers: I personally have a great distaste for these overrides-based eslint configs.

At our company we use multiple root eslint configs per package with great success. This works especially well, when you have browser TypeScript and Node JavaScript files in the same pacakge.

We might want to use a setup like the one suggested in @clark/eslint-config-ember in the future.

buschtoens · 2020-03-01T10:20:39Z

addon-test-support/-private/make-intl-helper.ts

+export default function makeIntlHelper<T, A1, A2>(
+  fn: (intl: IntlService, arg1: A1, arg2: A2) => T
+): (arg1: A1, arg2: A2) => T;


Does this work?

export default function makeIntlHelper<T, A extends unknown[]>( fn: (intl: IntlService, ...args: A) => T ): (...args: A) => T;

buschtoens · 2020-03-01T10:23:12Z

addon-test-support/-private/pick-last-locale.ts

+export default function pickLastLocale(locale: string | string[]) {
+  if (Array.isArray(locale)) {
+    return locale[locale.length - 1];
+  }
+
+  return locale;
+}


Some might say an arrow function would be easier to read:

export default (locale: string | string[]) => Array.isArray(locale) ? locale[locale.length - 1] : locale;

But fine with me. 👍

buschtoens · 2020-03-01T10:24:03Z

addon-test-support/-private/serialize-translation.ts

@@ -9,7 +9,7 @@ import omit from 'lodash.omit';
 * @param {object} obj
 * @return {string}
 */
-const stringifyDeterministically = obj => JSON.stringify(obj, Object.keys(obj).sort());
+const stringifyDeterministically = (obj: Record<string, any>) => JSON.stringify(obj, Object.keys(obj).sort());


Suggested change

const stringifyDeterministically = (obj: Record<string, any>) => JSON.stringify(obj, Object.keys(obj).sort());

const stringifyDeterministically = (obj: Record<string, unknown>) => JSON.stringify(obj, Object.keys(obj).sort());

buschtoens · 2020-03-01T10:24:15Z

addon-test-support/-private/serialize-translation.ts

@@ -20,7 +20,7 @@ const stringifyDeterministically = obj => JSON.stringify(obj, Object.keys(obj).s
 * @param {string} subject
 * @return {string}
 */
-const replaceInterpolators = subject =>
+const replaceInterpolators = (subject: any) =>


Suggested change

const replaceInterpolators = (subject: any) =>

const replaceInterpolators = (subject: unknown) =>

buschtoens · 2020-03-01T10:25:10Z

addon-test-support/-private/serialize-translation.ts

@@ -68,4 +69,5 @@ export const serializeTranslation = (key, options) => `t:${key}:${stringifyOptio
 * @return {string}
 * @hide
 */
-export const missingMessage = (key, locales, options) => serializeTranslation(key, options);
+export const missingMessage = (key: string, _: string[], options?: Record<string, any>) =>


Suggested change

export const missingMessage = (key: string, _: string[], options?: Record<string, any>) =>

export const missingMessage = (key: string, _locales: string[], options?: Record<string, unknown>) =>

buschtoens · 2020-03-01T10:27:26Z

addon-test-support/setup-intl.ts

+interface Hooks {
+  beforeEach: (fn: Function) => any;
+}


I think ember-qunit already exposes NestedHooks.

ember-qunit defines NestedHooks, but from what I could tell it isn't exported

buschtoens · 2020-03-01T10:40:21Z

addon-test-support/setup-intl.ts

+interface ApplicationContext {
+  owner: any;
+  intl?: IntlService;
+}


import { TestContext } from "ember-test-helpers"; export interface IntlTestContext extends TestContext { intl: IntlService; }

This would allow consumers to do this in their tests:

import { setupIntl, IntlTestContext } from 'ember-intl/test-support'; module('foo', function() { setupIntl(this); test('foo', (this: IntlTestContext, assert) { this.intl.setLocale('en-US'); // ... }) });

buschtoens · 2020-03-01T10:47:49Z

addon-test-support/setup-intl.ts

+    hooks.beforeEach(() => addTranslations(locale, undefined));
  }

  if (translations) {
-    hooks.beforeEach(() => addTranslations(translations));
+    hooks.beforeEach(() => addTranslations(translations, undefined));


Are the explicit undefined arguments necessary?

I would expect the translations? to work correctly. Maybe it works with the suggested ...args change to makeIntlHelper? 🤔

export default makeIntlHelper((intl: IntlService, localeName: string | Translations, translations?: Translations) => { ... });

buschtoens · 2020-03-01T10:48:56Z

types/global.d.ts

+// Types for compiled templates
+declare module 'ember-intl/templates/*' {
+  import { TemplateFactory } from 'htmlbars-inline-precompile';
+  const tmpl: TemplateFactory;
+  export default tmpl;
+}


I think we don't need these types, as ember-intl has no templates.

the imported private SafeString failed when using ember:try

among other PR feedback fixes

kturney · 2020-03-01T21:34:33Z

Thanks for the feedback! I've

rebased on master
replaced most uses of any with unknown
bumped typescripty deps

jasonmit · 2020-04-23T07:39:34Z

I'm still interesting in landing TS support but this has fallen a bit behind. Let me know if you want to re-attempt this and I'll try and hold off on any large changes.

jasonmit requested a review from buschtoens January 16, 2020 22:16

jasonmit requested changes Jan 17, 2020

View reviewed changes

kturney commented Jan 18, 2020

View reviewed changes

kturney mentioned this pull request Jan 27, 2020

Assertion failing in Ember beta/canary attempted to remove a function listener #1112

Closed

2 tasks

jasonmit approved these changes Jan 28, 2020

View reviewed changes

kturney force-pushed the typescript branch from c3be00f to bfbb91b Compare January 29, 2020 17:51

kturney force-pushed the typescript branch from 2c7f06e to c60f054 Compare January 31, 2020 21:34

buschtoens reviewed Mar 1, 2020

View reviewed changes

kturney added 15 commits March 1, 2020 11:01

setup ember-cli-typescript

cfc1fc3

mv addon js files to ts files

1202356

add types to addon code

0cdacea

fix for older ember versions

e43576c

remove commented out log

193e75b

add types for translations

6509a29

format ifs around timer cancellation

ae48cf6

add types for formatter options

68d0b4a

use our own SafeString type

d81534c

the imported private SafeString failed when using ember:try

use Record utility type where possible

5504d56

add undefined as possible Translations value

d2851cd

update typescript deps

5ae09e1

move test-support js files to ts

2cac16b

add types for test-helpers

dffe78e

fix ember-cli-babel not being latest after rebase

ccea47d

replace anys with unknowns

a0fbd6a

among other PR feedback fixes

kturney force-pushed the typescript branch from c60f054 to a0fbd6a Compare March 1, 2020 21:09

upgrade deps

4bf3512

jasonmit force-pushed the master branch 3 times, most recently from 3651c86 to 631b699 Compare March 28, 2020 04:05

jasonmit closed this Apr 23, 2020


		set(this: OverridableProps & IntlService, _, localeName: LocaleName) {
		// console.log('set locale', localeName);

		@@ -0,0 +1,15 @@
		import { assert } from '@ember/debug';

		function assertIsString(val: any): asserts val is string {

	const stringifyDeterministically = (obj: Record<string, any>) => JSON.stringify(obj, Object.keys(obj).sort());
	const stringifyDeterministically = (obj: Record<string, unknown>) => JSON.stringify(obj, Object.keys(obj).sort());

	const replaceInterpolators = (subject: any) =>
	const replaceInterpolators = (subject: unknown) =>

	export const missingMessage = (key: string, _: string[], options?: Record<string, any>) =>
	export const missingMessage = (key: string, _locales: string[], options?: Record<string, unknown>) =>

Typescript #1106

Typescript #1106

Conversation

kturney commented Jan 16, 2020

kturney commented Jan 16, 2020

jasonmit commented Jan 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonmit commented Jan 17, 2020

kturney left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kturney commented Jan 29, 2020

jasonmit commented Jan 31, 2020

fabmiz commented Feb 21, 2020 • edited

buschtoens commented Feb 21, 2020

buschtoens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kturney commented Mar 1, 2020

jasonmit commented Apr 23, 2020

fabmiz commented Feb 21, 2020 •

edited