0
\$\begingroup\$

I'm working through these Koans: https://github.com/paytonrules/typescript.koans#readme

I'm on file 3, collections, the very first function foreach:

export interface Dictionary<T> {
  [index: string]: T;
}

interface ArrayForEachIteratee<T> {
  (value?: T, index?: number, collection?: Array<T>): any;
}

interface DictionaryForEachIteratee<T> {
  (value?: T, key?: string, collection?: Dictionary<T>): any;
}

/**
 * ### forEach
 * should iterate over all items of array or all properties of an object
 *
 * ## Examples
 *
 *  let collection = ["first", "second", "third"];
 *  let result = [];
 *  let iteratee = (value, index, collection) => result[index] = [index, value];
 *
 *  _.forEach(collection, iteratee); => result === [[0, 'first'], [1, 'second'], [2, 'thidrd']];
 *
 *  collection = {
 *    "0": "first",
 *    "1": "second",
 *    "2": "third"
 *  };
 *  result = [];
 *  iteratee = (value, index, collection) => result[index] = [index, value];
 *
 *  _.forEach(collection, iteratee); => result === [['0', 'first'], ['1', 'second'], ['2', 'thidrd']];
 *
 */
export function forEach() {
}

When I look at the test code, I can see that the first test sends it an array and a function, and the second test sends it a dictionary and a function:

describe("forEach", function () {
    context("when collection is an array", function () {
      it("should iterate over all items of array", function () {
        const collection = ["first", "second", "third"];
        const iteratee = sinon.spy();
        _.forEach(collection, iteratee);
        sinon.assert.calledWithExactly(iteratee, "first", 0, collection);
        sinon.assert.calledWithExactly(iteratee, "second", 1, collection);
        sinon.assert.calledWithExactly(iteratee, "third", 2, collection);
      });
    });

    context("when collection is an object", function () {
      it("should iterate over all items of object", function () {
        const collection: _.Dictionary<string> = {
          "0": "first",
          "1": "second",
          "2": "third"
        };
        const iteratee = sinon.spy();
        _.forEach(collection, iteratee);
        sinon.assert.calledWithExactly(iteratee, "first", "0", collection);
        sinon.assert.calledWithExactly(iteratee, "second", "1", collection);
        sinon.assert.calledWithExactly(iteratee, "third", "2", collection);
      });
    });
  });

So I set up my code to accept an array or a dictionary, and the corresponding array / dictionary callback for the second argument:

export function forEach<T>(collection: Array<T> | Dictionary<T>, iteratee: ArrayForEachIteratee<T> | DictionaryForEachIteratee<T>): void {
  if (Array.isArray(collection)) {
    collection.forEach(iteratee);
  } else {
    Object.keys(collection)
        .forEach(key => {
          const value = collection[key];
          iteratee(value, key, collection);
        });
  }  
}

My code passes the tests - I'm passing the right arguments to the right functions in the right ways - but I'm absolutely certain that I'm not doing it in the proper Typescript-y type-safe way. My editor is showing me lots of red.

Can I get some pointers on how an expert typescripter would gracefully handle this situation?

\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

Compilation errors

Here is the reason is why you got compilation errors. Compiler doesn't understands that if collection is Array<T>, then iteratee is ArrayForEachIteratee<T>.

We should help compiler to realize that. Unfortunately, it seems it's impossible to build such instruction to typescript without significant rework of code.

So instead we can leverage type-casting:

if (Array.isArray(collection)) {
    collection.forEach(iteratee as ArrayForEachIteratee<T>);
    return;
} 

Object.keys(collection)
    .forEach(key => {
        const value = collection[key];
        (iteratee as DictionaryForEachIteratee<T>)(value, key, collection);
    });

But this doesn't looks pretty. In typescript there are assertion functions. Let's build one, but without actual checks on type (because we don't need them & we can't perform then):

function forceTypeNarrow<TWanted>(value: any): asserts value is TWanted {}

Now if we will put forceTypeNarrow somewhere in code, then all next lines in scope will think that value has type TWanted.

Now we can rewrite contents of our forEach function:

if (Array.isArray(collection)) {
    forceTypeNarrow<ArrayForEachIteratee<T>>(iteratee);
    collection.forEach(iteratee);
    return;
} 

Object.keys(collection)
    .forEach(key => {
        const value = collection[key];
        forceTypeNarrow<DictionaryForEachIteratee<T>>(iteratee);
        iteratee(value, key, collection);
    });

Proper function declaration

Let's take a look on your function declaration

export function forEach<T>(collection: Array<T> | Dictionary<T>, iteratee: ArrayForEachIteratee<T> | DictionaryForEachIteratee<T>): void {

This declaration allows user-code to pass (collection: Array<T>, iteratee: DictionaryForEachIteratee<T>). But we don't want to allow that!

Let's leverage overloads. We will put one overload for Array<T>, second overload for Dictionary<T>. And typescript forces us to put third non-end-user-code overload for internal purposes.

export function forEach<T>(
    collection: Array<T>, 
    iteratee: ArrayForEachIteratee<T>
): void;
export function forEach<T>(
    collection: Dictionary<T>, 
    iteratee: DictionaryForEachIteratee<T>
): void;
export function forEach<T>(
    collection: Array<T> | Dictionary<T>, 
    iteratee: ArrayForEachIteratee<T> | DictionaryForEachIteratee<T>
): void {
   …
}

Proper formatting of header

Long lines are not good, because they force everyone to use scroll (in IDE, in PR or here on codereview).

In case if your function header is too long, you should put each parameter on new-line. That will make your code readable.

\$\endgroup\$
13
  • 1
    \$\begingroup\$ thanks, this all was extremely helpful! thank you! I couldn't find any of this forceTypeNarrow function online -- do you know if a similar pattern is commonly used by a different name? \$\endgroup\$
    – TKoL
    Aug 12, 2021 at 16:57
  • 1
    \$\begingroup\$ No, sorry. I just invented it for sake of clarity and never seen it before \$\endgroup\$ Aug 12, 2021 at 17:46
  • 1
    \$\begingroup\$ Hey, one more question: forceTypeNarrow<ArrayForEachIteratee<T>>(iteratee); vs iteratee = iteratee as <ArrayForEachIteratee<T> -- is there any reason to prefer one over the other in your opinion? \$\endgroup\$
    – TKoL
    Aug 19, 2021 at 10:21
  • 1
    \$\begingroup\$ "your code (i = i as T) will not change type of variable i" -- my editor seems to think it works, and it compiles okay. \$\endgroup\$
    – TKoL
    Aug 19, 2021 at 11:14
  • 1
    \$\begingroup\$ @TKoL wow, I was wrong. \$\endgroup\$ Aug 19, 2021 at 14:48

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.