RFC: Improve mocks handling, builtIn modules and more.

[maloguertin]

Hi! First of all I want to thank you for this package. We have a pretty hefty code base with barrel imports. 650+ test suites and 3600+ tests. By making some changes to your packages to handle our code base I was able to cut testing time from from ~15 minutes to ~3 minutes on our local runs. I saw some good impact on the CI runs too but I still need to make improvements (~20 minutes to ~11 minutes).

This PR is not perfect but I wanted to get some inputs before putting more work into it. As of now we are using patch-package to make it so our test suite works with your package.

here's a list of things I am trying to support:

• Support wild card import by having the new import named as the old one (from import * as * from './abc' to import * as abc from './abc'
• Add check for builtIn modules to skip transform of import inspired by https://github.com/FogelAI/babel-plugin-transform-barrels
• Allow skipping node modules entirely
• Improve mock path replacement to match transformed imports correctly

[gtsop]

Hello @maloguertin , thank you very much for this PR, I am so glad this plugin helped you.

I assume that all the modifications you've made are part of your working fork for your use case, is this correct?

I need to study this PR a lot because it seems you have made a lot of changes and i cannot pick up the overall theme of what you're doing. It would make me really happy if we could unbatch all the features and potential bug fixes you did and discuss MRs one by one.

Also, very crutial, you have added a lot of functionality without including the corresponding tests. I know the initial codebase wasn't ultra tested either, but if new functionality is too be added in a stable manner we need to add tests. For instance you said you wanted so support import * as * from './abc' witch is a case i didn't know existed (edit: it seems like i didn't understand the case well, please elaborate on this too), we should add a test case here:

https://github.com/gtsop/babel-jest-boost/blob/master/spec/import-rewrites.spec.jsm#L55

Allow skipping node modules entirely

Can you please elaborate on why you would want this? I want to understand your use case because in my understanding it is a good thing that this library tries to dig into the node_modules. If there is a legit use case we should surely include this.

---

Again, don't take all that as much as a pushback, it's just that you've bundled a lot of work together and I appreciate your effort a lot. Please work with me and let's split all the featues/fixes one by one so i can better understand them

Edit 2: I just saw your bultin modules code, are you running this a node.js codebase? This plugin was created with the browser js in mind. Of course i would love if it worked for every environment, i just need to understand what you're working with here.

[maloguertin]

Hey! I am perfectly happy with your response. This was only to test the water as to how we should move forward with this, I was also planning to try to split this into smaller PRs.

Changes to wildcard import (avoid import as from ...)

Maybe I need to look into it again, but when I ran your plugin initially some imports we're being changed to this invalid format. I will need to check again if my fix for that is still required.

Changes to single parameter mocks

Here most of my changes we're made because some of my tests would fail because the replacement of the mocked path path.node.arguments[0].value = resolved was not matching the replacement in the imports.

/utils/index.ts

export { default as parseName } from './parseName'

/some-test-file.test.ts

import { parseName } from './utils'

jest.mock('./utils')

parseName.mockReturnValue('abc')

Here the test would fail because parseName would not be mocked because the transformed file would replace the import to import parseName from 'absolute/path/to/utils/parseName (in commonJS) and the mock to jest.mock('absolute/path/to/utils')

Allow skipping node modules entirely

I guess this is mostly because this plugin can be a little hard to get working so this is a way to easily start making it working without having to worry about transforms of module imports breaking your tests (as far as I know transformIgnorePattern does not work on resolved paths so you canno't simply put a node_modules regex there.

Also I think I had some problems with some node modules conflicting with es6 and commonJS although I should check that again. Plus for us, most modules we import are relatively small compared to our codebase so most of the gains comes from our own code transformation.

Bultin modules code

Jest is ran in a node context and some of the modules we're not being resolved if I remember correctly, ex:

/test-file.test.ts

import fs from 'fs'

const stringContent = fs.readFile('./my-string.txt')

the transform of the test file would break because 'fs' is a built-in module.

[gtsop]

@maloguertin I just pushed a commit to supprt the node_modules, didn't expect you would answer instantly. Please let me know if this commit addresses your use case. You just have to add ignoreNodeModules: true, to your config

https://github.com/gtsop/babel-jest-boost/commit/f5d0f4c1544e66924bcb923c5812c7313fe66932

[gtsop]

Changes to wildcard import (avoid import as from ...)

Maybe I need to look into it again, but when I ran your plugin initially some imports we're being changed to this invalid format. I will need to check again if my fix for that is still required.

Please check and if you can isolate the bug case we can add a test case to ensure we catch it.

Changes to single parameter mocks ....

Ok I will have to think this through a bit. I had been considered only with jest.mock('path', impl) in my target codebase, that's why the miss. I'll read your changes again now with this context and come back.

Allow skipping node modules entirely

Ok makes sense. In the front-end world we have infinite bloat in the node_modules and that's why it makes sense to traverse them by default (i saw you liked to have them skipped by default). Please check version 0.1.16 for that.

Bultin modules code

Jest is ran in a node context and some of the modules we're not being resolved if I remember correctly, ex:

/test-file.test.ts

import fs from 'fs'

const stringContent = fs.readFile('./my-string.txt')

the transform of the test file would break because 'fs' is a built-in module.

Ok so this means you are testing node.js code, not browser code. It makes sense that these modules should be ignored, i'll have a look at your code again, it looked legit, i just need to check if there is a node-way of deterministically understanding whether a module is bultin so that we don't maintain a list.

[maloguertin]

Okay I traced back the reason behind the wildcard import changes! Did you ever try your package alongside the @babel/preset-env preset?

example: /test_tree/core/index.js

export * as actionTypes from "./actionTypes";

/test_tree/core/actionTypes.js

export const x = 'my-string'

if I run transform on this string:

import { actionTypes as coreTypes } from './test_tree/core'; coreTypes.x;

it outputs:

"use strict";
var _actionTypes = require(".../babel-jest-boost/spec/test_tree/core/actionTypes.js");
_actionTypes["*"].x

whereas with my changes it outputs:

"use strict";
...(babel requires)
var coreTypes = _interopRequireWildcard(require(".../babel-jest-boost/spec/test_tree/core/actionTypes.js"));

function _getRequireWildcardCache(e) { if ("function" != typeof WeakMap) return null; var r = new WeakMap(), t = new WeakMap(); return (_getRequireWildcardCache = function _getRequireWildcardCache(e) { return e ? t : r; })(e); }
function _interopRequireWildcard(e, r) { if (!r && e && e.__esModule) return e; if (null === e || "object" != _typeof(e) && "function" != typeof e) return { "default": e }; var t = _getRequireWildcardCache(r); if (t && t.has(e)) return t.get(e); var n = { __proto__: null }, a = Object.defineProperty && Object.getOwnPropertyDescriptor; for (var u in e) if ("default" !== u && {}.hasOwnProperty.call(e, u)) { var i = a ? Object.getOwnPropertyDescriptor(e, u) : null; i && (i.get || i.set) ? Object.defineProperty(n, u, i) : n[u] = e[u]; } return n["default"] = e, t && t.set(e, n), n; }

coreTypes.x

edit: see https://github.com/gtsop/babel-jest-boost/pull/3

[maloguertin]

I think we can close this PR?