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

Migrate expect to typescript #7919

Merged
merged 48 commits into from Feb 20, 2019
Merged

Migrate expect to typescript #7919

merged 48 commits into from Feb 20, 2019

Conversation

natealcedo
Copy link
Contributor

@natealcedo natealcedo commented Feb 17, 2019

This PR migrates expect to typescript as part of #7807

A few points to note.

  1. The diff of before and after migration is too long so I've added in a diff file ts-migration.diff. You'll have to use the local git tool to view the diff since even github wont display it.
  2. I have a couple of ts-ignores since I didnt want to change the logic of the code.
  3. This PR is still a bit raw in my opinion. I'm more than happy to take feedback to improve it so do let me know!

Built diff:

diff --git c/packages/expect/build/asymmetricMatchers.js w/packages/expect/build/asymmetricMatchers.js
index 8e56ab67e..365cee7e4 100644
--- c/packages/expect/build/asymmetricMatchers.js
+++ w/packages/expect/build/asymmetricMatchers.js
@@ -12,8 +12,9 @@ var _utils = require('./utils');
 var Symbol = global['jest-symbol-do-not-touch'] || global.Symbol;
 
 class AsymmetricMatcher {
-  constructor() {
+  constructor(sample) {
     this.$$typeof = Symbol.for('jest.asymmetricMatcher');
+    this.sample = sample;
   }
 }
 
@@ -21,8 +22,6 @@ exports.AsymmetricMatcher = AsymmetricMatcher;
 
 class Any extends AsymmetricMatcher {
   constructor(sample) {
-    super();
-
     if (typeof sample === 'undefined') {
       throw new TypeError(
         'any() expects to be passed a constructor function. ' +
@@ -30,7 +29,7 @@ class Any extends AsymmetricMatcher {
       );
     }
 
-    this.sample = sample;
+    super(sample);
   }
 
   asymmetricMatch(other) {
@@ -106,8 +105,7 @@ class Anything extends AsymmetricMatcher {
 
 class ArrayContaining extends AsymmetricMatcher {
   constructor(sample, inverse = false) {
-    super();
-    this.sample = sample;
+    super(sample);
     this.inverse = inverse;
   }
 
@@ -140,8 +138,7 @@ class ArrayContaining extends AsymmetricMatcher {
 
 class ObjectContaining extends AsymmetricMatcher {
   constructor(sample, inverse = false) {
-    super();
-    this.sample = sample;
+    super(sample);
     this.inverse = inverse;
   }
 
@@ -192,13 +189,11 @@ class ObjectContaining extends AsymmetricMatcher {
 
 class StringContaining extends AsymmetricMatcher {
   constructor(sample, inverse = false) {
-    super();
-
     if (!(0, _jasmineUtils.isA)('String', sample)) {
       throw new Error('Expected is not a string');
     }
 
-    this.sample = sample;
+    super(sample);
     this.inverse = inverse;
   }
 
@@ -219,8 +214,6 @@ class StringContaining extends AsymmetricMatcher {
 
 class StringMatching extends AsymmetricMatcher {
   constructor(sample, inverse = false) {
-    super();
-
     if (
       !(0, _jasmineUtils.isA)('String', sample) &&
       !(0, _jasmineUtils.isA)('RegExp', sample)
@@ -228,7 +221,7 @@ class StringMatching extends AsymmetricMatcher {
       throw new Error('Expected is not a String or a RegExp');
     }
 
-    this.sample = new RegExp(sample);
+    super(new RegExp(sample));
     this.inverse = inverse;
   }
 
diff --git c/packages/expect/build/extractExpectedAssertionsErrors.js w/packages/expect/build/extractExpectedAssertionsErrors.js
index 4f07fb2c5..e709404ce 100644
--- c/packages/expect/build/extractExpectedAssertionsErrors.js
+++ w/packages/expect/build/extractExpectedAssertionsErrors.js
@@ -15,7 +15,6 @@ var _jestMatchersObject = require('./jestMatchersObject');
  * This source code is licensed under the MIT license found in the
  * LICENSE file in the root directory of this source tree.
  *
- *
  */
 const resetAssertionsLocalState = () => {
   (0, _jestMatchersObject.setState)({
diff --git c/packages/expect/build/fakeChalk.js w/packages/expect/build/fakeChalk.js
index 421c0c4f5..4e896b374 100644
--- c/packages/expect/build/fakeChalk.js
+++ w/packages/expect/build/fakeChalk.js
@@ -11,7 +11,6 @@ function _interopRequireDefault(obj) {
  *
  * This source code is licensed under the MIT license found in the
  * LICENSE file in the root directory of this source tree.
- *
  */
 const returnInput = str => str;
 
diff --git c/packages/expect/build/jasmineUtils.js w/packages/expect/build/jasmineUtils.js
index 62f3e62b1..46739a7db 100644
--- c/packages/expect/build/jasmineUtils.js
+++ w/packages/expect/build/jasmineUtils.js
@@ -33,7 +33,6 @@ LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION
 OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION
 WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
 
-
 */
 
 /* eslint-disable */
diff --git c/packages/expect/build/jestMatchersObject.js w/packages/expect/build/jestMatchersObject.js
index a4fc8b8be..787cf7a89 100644
--- c/packages/expect/build/jestMatchersObject.js
+++ w/packages/expect/build/jestMatchersObject.js
@@ -55,9 +55,8 @@ const setMatchers = (matchers, isInternal, expect) => {
       // expect is defined
       class CustomMatcher extends _asymmetricMatchers.AsymmetricMatcher {
         constructor(inverse = false, ...sample) {
-          super();
+          super(sample);
           this.inverse = inverse;
-          this.sample = sample;
         }
 
         asymmetricMatch(other) {
diff --git c/packages/expect/build/matchers.js w/packages/expect/build/matchers.js
index 301f3768a..f3781eb39 100644
--- c/packages/expect/build/matchers.js
+++ w/packages/expect/build/matchers.js
@@ -25,7 +25,6 @@ function _interopRequireDefault(obj) {
  * This source code is licensed under the MIT license found in the
  * LICENSE file in the root directory of this source tree.
  *
- *
  */
 const matchers = {
   toBe(received, expected) {
@@ -78,7 +77,7 @@ const matchers = {
   },
 
   toBeCloseTo(received, expected, precision = 2) {
-    const secondArgument = arguments.length === 3 ? 'precision' : null;
+    const secondArgument = arguments.length === 3 ? 'precision' : undefined;
     const isNot = this.isNot;
     const options = {
       isNot,
diff --git c/packages/expect/build/spyMatchers.js w/packages/expect/build/spyMatchers.js
index a79d55aac..1e552b0cb 100644
--- c/packages/expect/build/spyMatchers.js
+++ w/packages/expect/build/spyMatchers.js
@@ -54,14 +54,6 @@ function _arrayWithHoles(arr) {
   if (Array.isArray(arr)) return arr;
 }
 
-/**
- * Copyright (c) Facebook, Inc. and its affiliates. All Rights Reserved.
- *
- * This source code is licensed under the MIT license found in the
- * LICENSE file in the root directory of this source tree.
- *
- *
- */
 const CALL_PRINT_LIMIT = 3;
 const RETURN_PRINT_LIMIT = 5;
 const LAST_CALL_PRINT_LIMIT = 1;
@@ -402,7 +394,7 @@ const createNthCalledWithMatcher = matcherName => (
 ) => {
   ensureMock(received, matcherName);
   const receivedIsSpy = isSpy(received);
-  const type = receivedIsSpy ? 'spy' : 'mock function';
+  const type = receivedIsSpy ? 'spy' : 'mock function'; // @ts-ignore
 
   if (typeof nth !== 'number' || parseInt(nth, 10) !== nth || nth < 1) {
     const message = () =>
@@ -459,7 +451,7 @@ const createNthReturnedWithMatcher = matcherName => (
   nth,
   expected
 ) => {
-  ensureMock(received, matcherName);
+  ensureMock(received, matcherName); //@ts-ignore
 
   if (typeof nth !== 'number' || parseInt(nth, 10) !== nth || nth < 1) {
     const message = () =>
diff --git c/packages/expect/build/toThrowMatchers.js w/packages/expect/build/toThrowMatchers.js
index c9db3095b..9f00c5cbb 100644
--- c/packages/expect/build/toThrowMatchers.js
+++ w/packages/expect/build/toThrowMatchers.js
@@ -17,7 +17,6 @@ var _utils = require('./utils');
  * This source code is licensed under the MIT license found in the
  * LICENSE file in the root directory of this source tree.
  *
- *
  */
 const DID_NOT_THROW = 'Received function did not throw';
 
@@ -382,6 +381,7 @@ const formatStack = thrown =>
   thrown === null || !thrown.isError
     ? ''
     : (0, _jestMessageUtil.formatStackTrace)(
+        // @ts-ignore
         (0, _jestMessageUtil.separateMessageFromStack)(thrown.value.stack)
           .stack,
         {
diff --git c/packages/expect/build/types.js w/packages/expect/build/types.js
new file mode 100644
index 000000000..ad9a93a7c
--- /dev/null
+++ w/packages/expect/build/types.js
@@ -0,0 +1 @@
+'use strict';

@natealcedo natealcedo changed the title Migrate expect to typescript Migrate expect to typescript [WIP] Feb 17, 2019
Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Overall this looks pretty good, there's just a bit too much any 🙂 Seems like typing MatcherState correctly would resolve quite a bit of these issue

packages/expect/package.json Outdated Show resolved Hide resolved
packages/expect/src/asymmetricMatchers.ts Outdated Show resolved Hide resolved
packages/expect/src/asymmetricMatchers.ts Show resolved Hide resolved
@@ -146,14 +145,15 @@ class ArrayContaining extends AsymmetricMatcher {

class ObjectContaining extends AsymmetricMatcher {
sample: Object;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type this as {[key: string]: unknown} (or maybe any), and you shouldn't have to do the type casting later

packages/expect/src/asymmetricMatchers.ts Outdated Show resolved Hide resolved
packages/expect/src/fakeChalk.ts Outdated Show resolved Hide resolved
packages/expect/src/index.ts Outdated Show resolved Hide resolved
packages/expect/src/index.ts Outdated Show resolved Hide resolved
packages/expect/src/matchers.ts Show resolved Hide resolved
@SimenB SimenB added this to the TypeScript Migration milestone Feb 17, 2019
Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Great job, this was a tough one.

We can hopefully get rid of some of the anys at a future time, but since we know the code works it's no big deal

@SimenB SimenB changed the title Migrate expect to typescript [WIP] Migrate expect to typescript Feb 19, 2019
@codecov-io
Copy link

codecov-io commented Feb 20, 2019

Codecov Report

Merging #7919 into master will decrease coverage by 0.43%.
The diff coverage is 89.89%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7919      +/-   ##
==========================================
- Coverage   65.33%   64.89%   -0.44%     
==========================================
  Files         255      255              
  Lines        9941    10013      +72     
  Branches     1041     1361     +320     
==========================================
+ Hits         6495     6498       +3     
- Misses       3195     3199       +4     
- Partials      251      316      +65
Impacted Files Coverage Δ
...ages/expect/src/extractExpectedAssertionsErrors.ts 11.11% <ø> (ø)
packages/jest-snapshot/src/index.ts 31.25% <ø> (ø) ⬆️
packages/expect/src/fakeChalk.ts 100% <100%> (ø)
packages/expect/src/asymmetricMatchers.ts 86.45% <100%> (ø)
packages/expect/src/toThrowMatchers.ts 87.12% <100%> (ø)
packages/expect/src/jasmineUtils.ts 76.03% <100%> (ø)
packages/expect/src/matchers.ts 97.29% <100%> (ø)
packages/expect/src/utils.ts 95.31% <100%> (ø)
packages/expect/src/jestMatchersObject.ts 75% <72.72%> (ø)
packages/expect/src/spyMatchers.ts 87.43% <80%> (ø)
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b8a9a71...e14c798. Read the comment docs.

@natealcedo
Copy link
Contributor Author

Thanks @SimenB! I'm ready to pick up another one once this is good to go. Does this PR require a changelog update?

@SimenB
Copy link
Member

SimenB commented Feb 20, 2019

I'm ready to pick up another one once this is good to go.

Fantastic 😀

Does this PR require a changelog update?

It should! Pushed

Copy link
Collaborator

@thymikee thymikee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! There are still quite a bunch of any and casting we need to take care of later, but for now this is good enough :) I've added some small adjustments in the last commit

@@ -5,7 +5,7 @@
* LICENSE file in the root directory of this source tree.
*/

// TODO: Move this to `expect` when it's migrated
// TODO: Remove this when whole project is migrated
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cannot do it currently, because of export = stuff, and importing from expect/src/types seemed fishy (and caused TS errors, oh well who's got time for that XD)

@natealcedo
Copy link
Contributor Author

natealcedo commented Feb 20, 2019

Hey one question before this gets merged in. I was wondering about using the type object (lowercase o). I just noticed I was the only one doing this. Every other package that's been migrated to typescript that uses the object type uses the type Object (Capital O)

I didnt use Capital O object because I was referring to this https://www.typescriptlang.org/docs/handbook/declaration-files/do-s-and-don-ts.html

It states this

Don’t ever use the types Number, String, Boolean, or Object. These types refer to non-primitive boxed objects that are almost never used appropriately in JavaScript code.
Instead of Object, use the non-primitive object type (added in TypeScript 2.2).

Am I doing this wrongly?

Edit: spelling

@SimenB
Copy link
Member

SimenB commented Feb 20, 2019

IMO we shouldn't use Object or object - it should probably be {[key: string]: unknown}

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants