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/transform to TypeScript #7918

Merged
merged 4 commits into from
Feb 17, 2019

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented Feb 16, 2019

Summary

diff --git c/packages/jest-transform/build/ScriptTransformer.js w/packages/jest-transform/build/ScriptTransformer.js
index c572879d3..9cc6180d3 100644
--- c/packages/jest-transform/build/ScriptTransformer.js
+++ w/packages/jest-transform/build/ScriptTransformer.js
@@ -115,10 +115,6 @@ function _slash() {
   return data;
 }
 
-var _package = require('../package.json');
-
-var _shouldInstrument = _interopRequireDefault(require('./shouldInstrument'));
-
 function _writeFileAtomic() {
   const data = _interopRequireDefault(require('write-file-atomic'));
 
@@ -139,6 +135,8 @@ function _realpathNative() {
   return data;
 }
 
+var _shouldInstrument = _interopRequireDefault(require('./shouldInstrument'));
+
 var _enhanceUnexpectedTokenMessage = _interopRequireDefault(
   require('./enhanceUnexpectedTokenMessage')
 );
@@ -152,11 +150,14 @@ 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.
- *
- *
  */
-// This data structure is used to avoid recalculating some data every time that
+// @ts-ignore: should just be `require.resolve`, but the tests mess that up
+// Use `require` to avoid TS rootDir
+const _require = require('../package.json'),
+  VERSION = _require.version; // This data structure is used to avoid recalculating some data every time that
 // we need to transform a file. Since ScriptTransformer is instantiated for each
+// file we need to keep this object in the local scope of this module.
+
 const projectCaches = new WeakMap(); // To reset the cache for specific changesets (rather than package version).
 
 const CACHE_VERSION = '1';
@@ -208,10 +209,11 @@ class ScriptTransformer {
   }
 
   _getFileCachePath(filename, content, instrument) {
+    // @ts-ignore: not properly exported (needs ESM)
     const baseCacheDir = _jestHasteMap().default.getCacheFilePath(
       this._config.cacheDirectory,
       'jest-transform-cache-' + this._config.name,
-      _package.version
+      VERSION
     );
 
     const cacheKey = this._getCacheKey(content, filename, instrument); // Create sub folders based on the cacheKey to avoid creating one
@@ -245,7 +247,7 @@ class ScriptTransformer {
   }
 
   _getTransformer(filename) {
-    let transform;
+    let transform = null;
 
     if (!this._config.transform || !this._config.transform.length) {
       return null;
@@ -258,7 +260,7 @@ class ScriptTransformer {
 
       if (transformer != null) {
         return transformer;
-      } // $FlowFixMe
+      }
 
       transform = require(transformPath);
 
@@ -282,6 +284,7 @@ class ScriptTransformer {
     const result = (0, _core().transformSync)(content, {
       auxiliaryCommentBefore: ' istanbul ignore next ',
       babelrc: false,
+      // @ts-ignore: https://github.com/DefinitelyTyped/DefinitelyTyped/pull/33118
       caller: {
         name: '@jest/transform',
         supportsStaticESM: false
@@ -301,7 +304,16 @@ class ScriptTransformer {
         ]
       ]
     });
-    return result ? result.code : content;
+
+    if (result) {
+      const code = result.code;
+
+      if (code) {
+        return code;
+      }
+    }
+
+    return content;
   }
 
   _getRealPath(filepath) {
diff --git c/packages/jest-transform/build/shouldInstrument.js w/packages/jest-transform/build/shouldInstrument.js
index 7eca95eee..0f446d28a 100644
--- c/packages/jest-transform/build/shouldInstrument.js
+++ w/packages/jest-transform/build/shouldInstrument.js
@@ -54,8 +54,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 MOCKS_PATTERN = new RegExp(
   (0, _jestRegexUtil().escapePathForRegex)(
@@ -78,7 +76,7 @@ function shouldInstrument(filename, options, config) {
 
   if (
     !config.testPathIgnorePatterns ||
-    !config.testPathIgnorePatterns.some(pattern => filename.match(pattern))
+    !config.testPathIgnorePatterns.some(pattern => !!filename.match(pattern))
   ) {
     if (
       config.testRegex &&
@@ -124,7 +122,7 @@ function shouldInstrument(filename, options, config) {
 
   if (
     config.coveragePathIgnorePatterns &&
-    config.coveragePathIgnorePatterns.some(pattern => filename.match(pattern))
+    config.coveragePathIgnorePatterns.some(pattern => !!filename.match(pattern))
   ) {
     return false;
   }

Test plan

Green CI

@SimenB SimenB added this to the TypeScript Migration milestone Feb 16, 2019
@SimenB SimenB force-pushed the ts-jest-transformer branch 2 times, most recently from 92e3d03 to d1c361c Compare February 16, 2019 23:29
@codecov-io
Copy link

codecov-io commented Feb 16, 2019

Codecov Report

Merging #7918 into master will decrease coverage by 0.07%.
The diff coverage is 86.95%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7918      +/-   ##
==========================================
- Coverage   65.49%   65.42%   -0.08%     
==========================================
  Files         255      255              
  Lines        9930     9949      +19     
  Branches      965     1038      +73     
==========================================
+ Hits         6504     6509       +5     
- Misses       3192     3193       +1     
- Partials      234      247      +13
Impacted Files Coverage Δ
...est-transform/src/enhanceUnexpectedTokenMessage.ts 33.33% <ø> (ø)
packages/jest-transform/src/shouldInstrument.ts 95.34% <100%> (ø)
packages/babel-jest/src/index.ts 50% <100%> (-1.52%) ⬇️
packages/jest-transform/src/ScriptTransformer.ts 81.28% <84.21%> (ø)

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 d2023f1...d1dfdd5. Read the comment docs.

@SimenB SimenB force-pushed the ts-jest-transformer branch from 6cb7e7a to 6809772 Compare February 17, 2019 00:11
@SimenB SimenB requested a review from thymikee February 17, 2019 00:12
@SimenB SimenB force-pushed the ts-jest-transformer branch from b8f59c8 to 31718fd Compare February 17, 2019 00:19
@@ -22,12 +23,18 @@ const THIS_FILE = fs.readFileSync(__filename);
const jestPresetPath = require.resolve('babel-preset-jest');
const babelIstanbulPlugin = require.resolve('babel-plugin-istanbul');

// Narrow down the types
Copy link
Member Author

@SimenB SimenB Feb 17, 2019

Choose a reason for hiding this comment

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

quite happy with how this turned out. By using an interface, we can narrow down the type, and it'll tell us here if we break Transform's contract, and not just a local one. It also narrows down options of the factory to babel's options instead of any

And importers of babel-jest directly will see that createTransformer is always there - it's not possibly undefined. Pretty nice!

@@ -58,18 +65,15 @@ const createTransformer = (
return babelConfig;
}

return {
const transformer: BabelJestTransformer = {
Copy link
Member Author

@SimenB SimenB Feb 17, 2019

Choose a reason for hiding this comment

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

setting the type here allows us to not type all of the args explicitly

canInstrument: true,
createTransformer,
Copy link
Member Author

Choose a reason for hiding this comment

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

this was the solution to the FIXME below 🙂

Copy link
Contributor

@rubennorte rubennorte Feb 21, 2019

Choose a reason for hiding this comment

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

This broke all usages of createTransformer (vs using the transformer that's already created by Jest) because the object createTransformer returns also has a createTransformer property, which is checked in @babel/transform to use that instead of the passed one: https://github.com/facebook/jest/blob/master/packages/jest-transform/src/ScriptTransformer.ts#L157

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, oops! 😛 Good catch!

@SimenB SimenB force-pushed the ts-jest-transformer branch from 31718fd to 3f1dcd8 Compare February 17, 2019 00:36
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.

👌

let scriptCacheKey = null;
let instrument = false;
let result = '';
let result: TransformResult | string | undefined = '';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can it be really undefined?

Copy link
Member Author

@SimenB SimenB Feb 17, 2019

Choose a reason for hiding this comment

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

Yes, due to this._cache.transformedFiles.get(scriptCacheKey) below. We cannot return undefined from this method though.

But your comment made me look, and we don't actually return strings either, so was able to make it a bit stricter

@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.

5 participants