jest-extended icon indicating copy to clipboard operation
jest-extended copied to clipboard

Convert to TS project

Open igorwessel opened this issue 3 years ago • 8 comments

I hope that with this pull-request I'm not getting in the way of anyone working on it

What

  • Convert all files to TS
  • Add eslint for TS

Why

I believe it will be easier to add new types by letting the typescript handle the part of creating the .d.ts files. It will make it easier to maintain if jest happens to made a breaking change, we will be able to track this easier because of the types from jest.

Notes

  • I didn't remove the babel, I don't know if it will still be used maybe after the typescript compiles.
  • Currently I'm using the unknown type for the actual value, because we have no way to be sure/validate at runtime if the type that will be received will be what we decided in the matcher.

Housekeeping

  • [ ] Unit tests
  • [ ] Documentation is up to date
  • [ ] No additional lint warnings
  • [ ] Typescript definitions are added/updated where relevant

igorwessel avatar Mar 10 '22 09:03 igorwessel

I started looking at this too. I used this script to do the bulk rename. You might find it useful.

#!/usr/bin/env bash
set -o errexit -o pipefail -o nounset

function moveFiles() {
  dir="${1}"
  cd "${dir}"
  echo "Renaming files in ${PWD}"
  if compgen -G "*.js" > /dev/null; then
    for file in *.js; do
      git mv "${file}" "${file%.js}.ts"
    done
  fi
  if compgen -G "*.js.snap" > /dev/null; then
    for file in *.js.snap; do
      git mv "${file}" "${file%.js.snap}.ts.snap"
    done
  fi
  cd - > /dev/null
}

moveFiles .
moveFiles src
moveFiles src/all
moveFiles src/matchers
moveFiles src/utils
moveFiles test/matchers
moveFiles test/utils
moveFiles test/matchers/__snapshots__

keeganwitt avatar Mar 15 '22 05:03 keeganwitt

I started looking at this too. I used this script to do the bulk rename. You might find it useful.

#!/usr/bin/env bash
set -o errexit -o pipefail -o nounset

function moveFiles() {
  dir="${1}"
  cd "${dir}"
  echo "Renaming files in ${PWD}"
  if compgen -G "*.js" > /dev/null; then
    for file in *.js; do
      git mv "${file}" "${file%.js}.ts"
    done
  fi
  if compgen -G "*.js.snap" > /dev/null; then
    for file in *.js.snap; do
      git mv "${file}" "${file%.js.snap}.ts.snap"
    done
  fi
  cd - > /dev/null
}

moveFiles .
moveFiles src
moveFiles src/all
moveFiles src/matchers
moveFiles src/utils
moveFiles test/matchers
moveFiles test/utils
moveFiles test/matchers/__snapshots__

It will be very useful =D

igorwessel avatar Mar 15 '22 13:03 igorwessel

I'm thinking of putting validations in runtime for the actual one that is received via expect.

What do you think about it?

e.g (I took it from jest repo):

  if (typeof actual !== 'object' || actual === null) {
    throw new Error(
      matcherErrorMessage(
        matcherHint('toContainAnyValues'),
        `${RECEIVED_COLOR('received')} value must be a non-null object`,
        printWithType('Received', actual, printReceived),
      ),
    );
  }

igorwessel avatar Mar 17 '22 09:03 igorwessel

I'm thinking of putting validations in runtime for the actual one that is received via expect.

What do you think about it?

e.g (I took it from jest repo):

  if (typeof actual !== 'object' || actual === null) {
    throw new Error(
      matcherErrorMessage(
        matcherHint('toContainAnyValues'),
        `${RECEIVED_COLOR('received')} value must be a non-null object`,
        printWithType('Received', actual, printReceived),
      ),
    );
  }

Sorry for the late reply on this. Since, as you say, Jest's built-in matchers do runtime checks, so it seems reasonable to do it here as well.

keeganwitt avatar Apr 08 '22 04:04 keeganwitt

Sorry for the late reply on this. Since, as you say, Jest's built-in matchers do runtime checks, so it seems reasonable to do it here as well.

No problem. I'll start doing the checks this weekend.

igorwessel avatar Apr 08 '22 09:04 igorwessel

@keeganwitt @mattphillips

I needed an opinion before proceeding with the other files, I thought of doing the validations as follows.

And I'll probably only add the validations to matchers that do something with received value x expected value e.g (matchers not worth doing validation): toBeArray, toBeDate, etc...

made a type-guard function to narrow down type and throw a error

// src/utils.ts
export function validateType<A>(condition: boolean, value: unknown, message: string): asserts value is A {
  if (!condition) {
    throw new Error(message);
  }
}

// src/toBeAfter (for example)
``ts
export function toBeAfter(this: jest.MatcherContext, actual: unknown, expected: Date): jest.CustomMatcherResult {
  const {
    printReceived,
    printExpected,
    matcherHint,
    matcherErrorMessage,
    RECEIVED_COLOR,
    EXPECTED_COLOR,
    printWithType,
  } = this.utils;
  const matcherName = 'toBeAfter';

  validateType<Date>(
    actual instanceof Date,
    actual,
    matcherErrorMessage(
      matcherHint(matcherName),
      `${RECEIVED_COLOR('received')} value must be a Date.`,
      printWithType('Received', actual, printReceived),
    ),
  );

 // not need pass type parameter because expected is typed.
  validateType(
    expected instanceof Date,
    expected,
    matcherErrorMessage(
      matcherHint(matcherName),
      `${EXPECTED_COLOR('expected')} value must be a Date.`,
      printWithType('Expected', expected, printExpected),
    ),
  );

  const passMessage =
    matcherHint(`.not.${matcherName}`, 'received', '') +
    '\n\n' +
    `Expected date to be after ${printReceived(expected)} but received:\n` +
    `  ${printReceived(actual)}`;

  const failMessage =
    matcherHint(`.${matcherName}`, 'received', '') +
    '\n\n' +
    `Expected date to be after ${printReceived(expected)} but received:\n` +
    `  ${printReceived(actual)}`;

  const pass = actual > expected;

  return { pass, message: () => (pass ? passMessage : failMessage) };
}
``

igorwessel avatar Apr 09 '22 09:04 igorwessel

Regarding this function I didn't really like the fact that I'm passing the value but currently I'm not doing anything with him. I only use it to get to be able to do narrow down type.

// src/utils.ts
export function validateType<A>(condition: boolean, value: unknown, message: string): asserts value is A {
  if (!condition) {
    throw new Error(message);
  }
}

Maybe separate several validation functions for each type to remove the parameter value? something in this way:

export function validateString(value: unknown, message: string): asserts value is string {
  if (typeof value !== 'string') {
    throw new Error(message);
  }
}

export function validateObject(value: unknown, message: string): asserts value is Record<string, unknown> {
  if (typeof value !== 'object' && value === null) {
    throw new Error(message);
  }
}

igorwessel avatar Apr 10 '22 09:04 igorwessel

I think I'd suggest maybe holding off on the validation for now and picking that up in a separate PR. We're not currently checking the types (or at least we're not for all matchers), so it's not functionality we're losing. Plus, adding the validations (where they didn't exist before) is I think maybe a breaking change, so it'd probably be nice to release that separately too.

keeganwitt avatar Apr 13 '22 03:04 keeganwitt