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
Collaborator

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 SimenB:ts-jest-transformer branch 2 times, most recently from 92e3d03 to d1c361c Feb 16, 2019
@codecov-io

This comment has been minimized.

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 SimenB:ts-jest-transformer branch from 6cb7e7a to 6809772 Feb 17, 2019
@SimenB SimenB requested a review from thymikee Feb 17, 2019
@SimenB SimenB force-pushed the SimenB:ts-jest-transformer branch from b8f59c8 to 31718fd Feb 17, 2019
@@ -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

This comment has been minimized.

Copy link
@SimenB

SimenB Feb 17, 2019

Author Collaborator

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 = {

This comment has been minimized.

Copy link
@SimenB

SimenB Feb 17, 2019

Author Collaborator

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

canInstrument: true,
createTransformer,

This comment has been minimized.

Copy link
@SimenB

SimenB Feb 17, 2019

Author Collaborator

this was the solution to the FIXME below 🙂

This comment has been minimized.

Copy link
@rubennorte

rubennorte Feb 21, 2019

Contributor

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

This comment has been minimized.

Copy link
@SimenB

SimenB Feb 21, 2019

Author Collaborator

Oh, oops! 😛 Good catch!

SimenB added 2 commits Feb 16, 2019
…ypes
@SimenB SimenB force-pushed the SimenB:ts-jest-transformer branch from 31718fd to 3f1dcd8 Feb 17, 2019
Copy link
Collaborator

thymikee left a comment

👌

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

This comment has been minimized.

Copy link
@thymikee

thymikee Feb 17, 2019

Collaborator

Can it be really undefined?

This comment has been minimized.

Copy link
@SimenB

SimenB Feb 17, 2019

Author Collaborator

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

SimenB added 2 commits Feb 17, 2019
@SimenB SimenB merged commit 7ae3f07 into facebook:master Feb 17, 2019
12 checks passed
12 checks passed
ci/circleci: lint-and-typecheck Your tests passed on CircleCI!
Details
ci/circleci: test-browser Your tests passed on CircleCI!
Details
ci/circleci: test-jest-circus Your tests passed on CircleCI!
Details
ci/circleci: test-node-10 Your tests passed on CircleCI!
Details
ci/circleci: test-node-11 Your tests passed on CircleCI!
Details
ci/circleci: test-node-6 Your tests passed on CircleCI!
Details
ci/circleci: test-node-8 Your tests passed on CircleCI!
Details
ci/circleci: test-or-deploy-website Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
facebook.jest #20190217.16 succeeded
Details
@SimenB SimenB deleted the SimenB:ts-jest-transformer branch Feb 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.