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

chore: migrate jest-config to TypeScript #7944

Merged
merged 8 commits into from
Feb 26, 2019
Merged

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented Feb 21, 2019

Summary

This isn't ready for merge (of course I'm having issues with normalize...) Help greatly appreciated!

I've purposefully not moved the config types into this package, even though it uses ESM so we would be able to re-export. The reason being the huge dependency tree of jest-config that would be unfortunate to impose on all consumers. Can do something about that after #7738.

Built diff:

diff --git c/packages/jest-config/build/index.js w/packages/jest-config/build/index.js
index 2b3aab25a..8e0049751 100644
--- c/packages/jest-config/build/index.js
+++ w/packages/jest-config/build/index.js
@@ -48,30 +48,30 @@ Object.defineProperty(exports, 'descriptions', {
   }
 });
 
-function _chalk() {
-  const data = _interopRequireDefault(require('chalk'));
+function _fs() {
+  const data = _interopRequireDefault(require('fs'));
 
-  _chalk = function _chalk() {
+  _fs = function _fs() {
     return data;
   };
 
   return data;
 }
 
-function _fs() {
-  const data = _interopRequireDefault(require('fs'));
+function _path() {
+  const data = _interopRequireDefault(require('path'));
 
-  _fs = function _fs() {
+  _path = function _path() {
     return data;
   };
 
   return data;
 }
 
-function _path() {
-  const data = _interopRequireDefault(require('path'));
+function _chalk() {
+  const data = _interopRequireDefault(require('chalk'));
 
-  _path = function _path() {
+  _chalk = function _chalk() {
     return data;
   };
 
@@ -103,8 +103,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.
- *
- *
  */
 function readConfig(
   argv,
diff --git c/packages/jest-config/build/normalize.js w/packages/jest-config/build/normalize.js
index 2b0fa4c56..303859c86 100644
--- c/packages/jest-config/build/normalize.js
+++ w/packages/jest-config/build/normalize.js
@@ -617,27 +617,33 @@ function normalize(options, argv, configPath, projectIndex = Infinity) {
       case 'setupFiles':
       case 'setupFilesAfterEnv':
       case 'snapshotSerializers':
-        value =
-          options[key] &&
-          options[key].map(filePath =>
-            (0, _utils.resolve)(newOptions.resolver, {
-              filePath,
-              key,
-              rootDir: options.rootDir
-            })
-          );
+        {
+          const option = options[key];
+          value =
+            option &&
+            option.map(filePath =>
+              (0, _utils.resolve)(newOptions.resolver, {
+                filePath,
+                key,
+                rootDir: options.rootDir
+              })
+            );
+        }
         break;
 
       case 'modulePaths':
       case 'roots':
-        value =
-          options[key] &&
-          options[key].map(filePath =>
-            _path().default.resolve(
-              options.rootDir,
-              (0, _utils.replaceRootDirInPath)(options.rootDir, filePath)
-            )
-          );
+        {
+          const option = options[key];
+          value =
+            option &&
+            option.map(filePath =>
+              _path().default.resolve(
+                options.rootDir,
+                (0, _utils.replaceRootDirInPath)(options.rootDir, filePath)
+              )
+            );
+        }
         break;
 
       case 'collectCoverageFrom':
@@ -646,12 +652,15 @@ function normalize(options, argv, configPath, projectIndex = Infinity) {
 
       case 'cacheDirectory':
       case 'coverageDirectory':
-        value =
-          options[key] &&
-          _path().default.resolve(
-            options.rootDir,
-            (0, _utils.replaceRootDirInPath)(options.rootDir, options[key])
-          );
+        {
+          const option = options[key];
+          value =
+            option &&
+            _path().default.resolve(
+              options.rootDir,
+              (0, _utils.replaceRootDirInPath)(options.rootDir, option)
+            );
+        }
         break;
 
       case 'dependencyExtractor':
@@ -662,36 +671,45 @@ function normalize(options, argv, configPath, projectIndex = Infinity) {
       case 'testResultsProcessor':
       case 'testRunner':
       case 'filter':
-        value =
-          options[key] &&
-          (0, _utils.resolve)(newOptions.resolver, {
-            filePath: options[key],
-            key,
-            rootDir: options.rootDir
-          });
+        {
+          const option = options[key];
+          value =
+            option &&
+            (0, _utils.resolve)(newOptions.resolver, {
+              filePath: option,
+              key,
+              rootDir: options.rootDir
+            });
+        }
         break;
 
       case 'runner':
-        value =
-          options[key] &&
-          (0, _utils.getRunner)(newOptions.resolver, {
-            filePath: options[key],
-            rootDir: options.rootDir
-          });
+        {
+          const option = options[key];
+          value =
+            option &&
+            (0, _utils.getRunner)(newOptions.resolver, {
+              filePath: option,
+              rootDir: options.rootDir
+            });
+        }
         break;
 
       case 'prettierPath':
-        // We only want this to throw if "prettierPath" is explicitly passed
-        // from config or CLI, and the requested path isn't found. Otherwise we
-        // set it to null and throw an error lazily when it is used.
-        value =
-          options[key] &&
-          (0, _utils.resolve)(newOptions.resolver, {
-            filePath: options[key],
-            key,
-            optional: options[key] === _Defaults.default[key],
-            rootDir: options.rootDir
-          });
+        {
+          // We only want this to throw if "prettierPath" is explicitly passed
+          // from config or CLI, and the requested path isn't found. Otherwise we
+          // set it to null and throw an error lazily when it is used.
+          const option = options[key];
+          value =
+            option &&
+            (0, _utils.resolve)(newOptions.resolver, {
+              filePath: option,
+              key,
+              optional: option === _Defaults.default[key],
+              rootDir: options.rootDir
+            });
+        }
         break;
 
       case 'moduleNameMapper':
@@ -784,9 +802,14 @@ function normalize(options, argv, configPath, projectIndex = Infinity) {
         break;
 
       case 'testRegex':
-        value = options[key]
-          ? [].concat(options[key]).map(_jestRegexUtil().replacePathSepForRegex)
-          : [];
+        {
+          const option = options[key];
+          value = option
+            ? (Array.isArray(option) ? option : [option]).map(
+                _jestRegexUtil().replacePathSepForRegex
+              )
+            : [];
+        }
         break;
 
       case 'moduleFileExtensions': {
@@ -820,14 +843,16 @@ function normalize(options, argv, configPath, projectIndex = Infinity) {
       }
 
       case 'bail': {
-        if (typeof options[key] === 'boolean') {
-          value = options[key] ? 1 : 0;
-        } else if (typeof options[key] === 'string') {
+        const bail = options[key];
+
+        if (typeof bail === 'boolean') {
+          value = bail ? 1 : 0;
+        } else if (typeof bail === 'string') {
           value = 1; // If Jest is invoked as `jest --bail someTestPattern` then need to
           // move the pattern from the `bail` configuration and into `argv._`
           // to be processed as an extra parameter
 
-          argv._.push(options[key]);
+          argv._.push(bail);
         } else {
           value = options[key];
         }
@@ -912,7 +937,7 @@ function normalize(options, argv, configPath, projectIndex = Infinity) {
           }
         });
         break;
-    } // $FlowFixMe - automock is missing in GlobalConfig, so what
+    } // @ts-ignore: automock is missing in GlobalConfig, so what
 
     newOptions[key] = value;
     return newOptions;
@@ -921,14 +946,13 @@ function normalize(options, argv, configPath, projectIndex = Infinity) {
   newOptions.testPathPattern = buildTestPathPattern(argv);
   newOptions.json = argv.json;
   newOptions.testFailureExitCode = parseInt(newOptions.testFailureExitCode, 10);
-  var _arr = ['lastCommit', 'changedFilesWithAncestor', 'changedSince'];
-
-  for (var _i = 0; _i < _arr.length; _i++) {
-    const key = _arr[_i];
 
-    if (newOptions[key]) {
-      newOptions.onlyChanged = true;
-    }
+  if (
+    newOptions.lastCommit ||
+    newOptions.changedFilesWithAncestor ||
+    newOptions.changedSince
+  ) {
+    newOptions.onlyChanged = true;
   }
 
   if (argv.all) {
diff --git c/packages/jest-config/build/utils.js w/packages/jest-config/build/utils.js
index 2a3960f98..9ae0774fd 100644
--- c/packages/jest-config/build/utils.js
+++ w/packages/jest-config/build/utils.js
@@ -78,7 +78,7 @@ const resolve = (resolver, {key, filePath, rootDir, optional}) => {
     replaceRootDirInPath(rootDir, filePath),
     {
       basedir: rootDir,
-      resolver
+      resolver: resolver || undefined
     }
   );
 
@@ -158,7 +158,7 @@ const resolveWithPrefix = (
 
   let module = _jestResolve().default.findNodeModule(`${prefix}${fileName}`, {
     basedir: rootDir,
-    resolver
+    resolver: resolver || undefined
   });
 
   if (module) {
@@ -171,7 +171,7 @@ const resolveWithPrefix = (
 
   module = _jestResolve().default.findNodeModule(fileName, {
     basedir: rootDir,
-    resolver
+    resolver: resolver || undefined
   });
 
   if (module) {

Test plan

Green CI (eventually)

@jeysal
Copy link
Contributor

jeysal commented Feb 24, 2019

jeysal@ce138f1 fixes most errors in not-too-ugly ways (but has some extracted variables that will cause changes in the build output).
There's two errors remaining that are, I think, really errors that can cause the normalize return value to not match its asserted type.
image
E.g. detectLeaks is not optional on AllOptions, but I think it might be missing on the returned value.
image
hasteImplModulePath is not allowed to be null, but it could become null here.

We should decide on some actual structural changes to make that fix especially the first one.

@SimenB
Copy link
Member Author

SimenB commented Feb 24, 2019

Thanks again! Cherry-picked it, and did a rebase

E.g. detectLeaks is not optional on AllOptions, but I think it might be missing on the returned value.

It should be set to false if it's not set from argv

hasteImplModulePath is not allowed to be null, but it could become null here.

Than should be allowed to be null (or undefined at least). I wonder if it's left over from #6104? Not sure, but it's definitely allowed to be null as Jest doesn't ship with an implementation and it's not required.

@jeysal
Copy link
Contributor

jeysal commented Feb 24, 2019

jeysal@b7caef6 brings DefaultConfig and ProjectConfig closer together, but there's more mismatch in existing keys than expected, so I didn't manage to finish it today. Also, I modified ProjectConfig to allow nullish for dependencyExtractor and moduleLoader, which I think can already have nullish values going out of normalize today, but you should double check.

@SimenB SimenB force-pushed the ts-jest-config branch 2 times, most recently from 2d59350 to 4ead970 Compare February 25, 2019 11:49
@SimenB
Copy link
Member Author

SimenB commented Feb 25, 2019

Rebased after merging #7964 which also added the Argv typings

@SimenB SimenB marked this pull request as ready for review February 25, 2019 20:17
@SimenB
Copy link
Member Author

SimenB commented Feb 25, 2019

@jeysal I just did 9f9d473 🤷‍♂️ Seems silly to spend too much time on it when we wanna rework the whole thing later. As longs as it works, I'm happy

@jeysal
Copy link
Contributor

jeysal commented Feb 25, 2019

@SimenB okay. At least the huge switch is almost properly typed 😄

@SimenB
Copy link
Member Author

SimenB commented Feb 25, 2019

If you feel up to it, I'd love a PR removing the cast

@@ -90,6 +89,13 @@ async function runTestInternal(
let testEnvironment = config.testEnvironment;

if (customEnvironment) {
if (Array.isArray(customEnvironment)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

I love it it when types catch a potential bug

@jeysal
Copy link
Contributor

jeysal commented Feb 25, 2019

Don't think it really makes sense to, when I tried to go through the mismatching keys it felt like I was just guessing which keys were allowed to be nullish going out of normalize etc...

@codecov-io
Copy link

Codecov Report

Merging #7944 into master will decrease coverage by 0.29%.
The diff coverage is 85.26%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #7944     +/-   ##
=========================================
- Coverage   64.54%   64.24%   -0.3%     
=========================================
  Files         257      258      +1     
  Lines       10074    10117     +43     
  Branches     1587     1742    +155     
=========================================
- Hits         6502     6500      -2     
- Misses       3229     3234      +5     
- Partials      343      383     +40
Impacted Files Coverage Δ
packages/jest-config/src/setFromArgv.ts 85.71% <ø> (ø)
packages/jest-config/src/validatePattern.ts 100% <ø> (ø)
...ckages/jest-config/src/ReporterValidationErrors.ts 11.76% <ø> (ø)
...ges/jest-config/src/readConfigFileAndSetRootDir.ts 0% <ø> (ø)
packages/jest-config/src/constants.ts 100% <ø> (ø)
packages/jest-config/src/getCacheDirectory.ts 66.66% <ø> (ø)
packages/jest-runner/src/runTest.ts 2.53% <0%> (-0.07%) ⬇️
packages/jest-config/src/getMaxWorkers.ts 100% <100%> (ø)
packages/jest-config/src/Defaults.ts 100% <100%> (ø)
packages/jest-config/src/resolveConfigPath.ts 90.9% <100%> (ø)
... and 12 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 51817fd...ffde232. Read the comment docs.

@SimenB SimenB merged commit 25133b3 into jestjs:master Feb 26, 2019
@SimenB SimenB deleted the ts-jest-config branch February 26, 2019 11:09
@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.

4 participants