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-resolve to TypeScript #7871

Merged
merged 1 commit into from
Feb 13, 2019

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented Feb 12, 2019

Summary

As expected, had to tweak jest-haste-map's types a bit to match undefined | null stuff. Can revisit later and be more consistent across the board

Built diff:

diff --git c/packages/jest-resolve/build/defaultResolver.js w/packages/jest-resolve/build/defaultResolver.js
index fab1abcb1..60db7374c 100644
--- c/packages/jest-resolve/build/defaultResolver.js
+++ w/packages/jest-resolve/build/defaultResolver.js
@@ -5,30 +5,30 @@ Object.defineProperty(exports, '__esModule', {
 });
 exports.default = defaultResolver;
 
-function _browserResolve() {
-  const data = _interopRequireDefault(require('browser-resolve'));
+function _fs() {
+  const data = _interopRequireDefault(require('fs'));
 
-  _browserResolve = function _browserResolve() {
+  _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 _browserResolve() {
+  const data = _interopRequireDefault(require('browser-resolve'));
 
-  _path = function _path() {
+  _browserResolve = function _browserResolve() {
     return data;
   };
 
@@ -48,8 +48,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 defaultResolver(path, options) {
   const resolve = options.browser
diff --git c/packages/jest-resolve/build/index.js w/packages/jest-resolve/build/index.js
index a397e5f2b..af3a8dfdb 100644
--- c/packages/jest-resolve/build/index.js
+++ w/packages/jest-resolve/build/index.js
@@ -20,12 +20,6 @@ function _realpathNative() {
   return data;
 }
 
-var _nodeModulesPaths = _interopRequireDefault(require('./nodeModulesPaths'));
-
-var _isBuiltinModule = _interopRequireDefault(require('./isBuiltinModule'));
-
-var _defaultResolver = _interopRequireDefault(require('./defaultResolver'));
-
 function _chalk() {
   const data = _interopRequireDefault(require('chalk'));
 
@@ -36,6 +30,12 @@ function _chalk() {
   return data;
 }
 
+var _nodeModulesPaths = _interopRequireDefault(require('./nodeModulesPaths'));
+
+var _isBuiltinModule = _interopRequireDefault(require('./isBuiltinModule'));
+
+var _defaultResolver = _interopRequireDefault(require('./defaultResolver'));
+
 function _interopRequireDefault(obj) {
   return obj && obj.__esModule ? obj : {default: obj};
 }
@@ -45,8 +45,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 NATIVE_PLATFORM = 'native'; // We might be inside a symlink.
 
diff --git c/packages/jest-resolve/build/isBuiltinModule.js w/packages/jest-resolve/build/isBuiltinModule.js
index 9ae6e31ef..a4c9f5035 100644
--- c/packages/jest-resolve/build/isBuiltinModule.js
+++ w/packages/jest-resolve/build/isBuiltinModule.js
@@ -5,29 +5,30 @@ Object.defineProperty(exports, '__esModule', {
 });
 exports.default = isBuiltinModule;
 
-function _module() {
-  const data = require('module');
+function _module2() {
+  const data = _interopRequireDefault(require('module'));
 
-  _module = function _module() {
+  _module2 = function _module2() {
     return data;
   };
 
   return data;
 }
 
+function _interopRequireDefault(obj) {
+  return obj && obj.__esModule ? obj : {default: obj};
+}
+
 /**
  * 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.
- *
- *
  */
-// $FlowFixMe: Flow doesn't know about the `module` module
 const EXPERIMENTAL_MODULES = ['worker_threads'];
 const BUILTIN_MODULES = new Set(
-  _module().builtinModules
-    ? _module().builtinModules.concat(EXPERIMENTAL_MODULES)
+  _module2().default.builtinModules
+    ? _module2().default.builtinModules.concat(EXPERIMENTAL_MODULES)
     : Object.keys(process.binding('natives'))
         .filter(module => !/^internal\//.test(module))
         .concat(EXPERIMENTAL_MODULES)
diff --git c/packages/jest-resolve/build/nodeModulesPaths.js w/packages/jest-resolve/build/nodeModulesPaths.js
index c3bef18cd..8d9d10f0c 100644
--- c/packages/jest-resolve/build/nodeModulesPaths.js
+++ w/packages/jest-resolve/build/nodeModulesPaths.js
@@ -36,13 +36,11 @@ function _interopRequireDefault(obj) {
  * LICENSE file in the root directory of this source tree.
  *
  * Adapted from: https://github.com/substack/node-resolve
- *
- *
  */
 function nodeModulesPaths(basedir, options) {
   const modules =
     options && options.moduleDirectory
-      ? [].concat(options.moduleDirectory)
+      ? Array.from(options.moduleDirectory)
       : ['node_modules']; // ensure that `basedir` is an absolute path at this point,
   // resolving against the process' current working directory
 
diff --git c/packages/jest-resolve/build/types.js w/packages/jest-resolve/build/types.js
new file mode 100644
index 000000000..ad9a93a7c
--- /dev/null
+++ w/packages/jest-resolve/build/types.js
@@ -0,0 +1 @@
+'use strict';

Test plan

Green CI

package.json Show resolved Hide resolved
},
"devDependencies": {
"@types/browser-resolve": "^0.0.5",
"jest-haste-map": "^24.0.0"
},
"peerDependencies": {
"jest-haste-map": "^24.0.0"
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 think this makes the most sense - the constructor of jest-resolve takes ModuleMap from jest-haste-map. So both for types but also for usage, we need to ensure that the API is what we expect it to be

@codecov-io
Copy link

Codecov Report

Merging #7871 into master will decrease coverage by 0.27%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7871      +/-   ##
==========================================
- Coverage   58.41%   58.14%   -0.28%     
==========================================
  Files         178      174       -4     
  Lines        6630     6424     -206     
  Branches        5        6       +1     
==========================================
- Hits         3873     3735     -138     
+ Misses       2755     2687      -68     
  Partials        2        2

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 2688c2d...b909b2d. Read the comment docs.

packages/jest-haste-map/src/ModuleMap.ts Show resolved Hide resolved
import userResolver from '../__mocks__/userResolver';
import nodeModulesPaths from '../nodeModulesPaths';
import defaultResolver from '../defaultResolver';
import {ResolverConfig} from '../types';

// @ts-ignore: types are wrong. not sure how...
Copy link
Contributor

Choose a reason for hiding this comment

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

built haste-map index.d.ts:

import HasteFS from './HasteFS';
import HasteModuleMap, { SerializableModuleMap as HasteSerializableModuleMap } from './ModuleMap';
export declare type ModuleMap = HasteModuleMap;
export declare type SerializableModuleMap = HasteSerializableModuleMap;
export declare type FS = HasteFS;

totally missing the main export.
Working on how to fix this atm

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure we can fix this without dropping the export types at the top of haste map. Basically, we want both export = HasteMap (because making it the default export like it should be would be breaking) and export type .... It would be fine for a normal property (can just put it on the class), but I don't know how we could possibly put types onto HasteMap

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for digging! I say we keep it as is then, and try to go for a real ESM run at some point in the future.

It's easier with modules like haste map anyways since it's mostly internal and not pluggable like other parts of Jest are

Copy link
Contributor

Choose a reason for hiding this comment

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

Guess the ModuleMap and HasteFS types are more important anyway, they're the ones used in Context etc

@SimenB SimenB requested a review from thymikee February 13, 2019 09:23
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.

Awesome

@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