Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convert to TS project #423

Draft
wants to merge 87 commits into
base: main
Choose a base branch
from

Conversation

igorwessel
Copy link
Contributor

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

@keeganwitt
Copy link
Collaborator

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
Copy link
Collaborator

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.

@igorwessel
Copy link
Contributor Author

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
Copy link
Contributor Author

igorwessel commented Apr 9, 2022

@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
Copy link
Contributor Author

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);
  }
}

@keeganwitt
Copy link
Collaborator

keeganwitt commented Apr 13, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants