Inline require

[SimenB]

Summary
This is WIP. I can keep working on it if the maintainers are interested 😄

Use babel-plugin-transform-inline-imports-commonjs to inline all requires, making most modules lazy-loaded. This should improve runtime performance.

This strategy was used som places in the code base already, this change ensures it happens for everything.

build dir is included in git to view how the code has changed (see second commit). Will of course remove it before merge.

Test plan

yarn jest should pass.

[thymikee]

Can you please bring back build to gitignore? It's hard to even scroll through so many lines and see what's changed in the source :(

[SimenB]

@thymikee Added a commit removing all build-files again 😄

[thymikee]

Thank you, it's much better now ❤️

[thymikee]

This is really great and I'm all for this change.
Some of the tests are failing, but I can see that's expected.

What's best – I ran yarn jest -- snapshot (that's the quickest I got to successfully run all test suites):

• master:

• this diff:

That's DOUBLE the speed, wow.
cc: @cpojer @zertosh

[cpojer]

I have the worst Internet here in Berlin at JSConf but this is super duper awesome

One thought: can we somehow verify that we always apply this optimization? I would like us to codemod all of Jest to imports at some point and use the inline-imports transform then but we also need to make sure that once we do that, we aren't breaking this and slowing Jest down a lot.

[cpojer]

@SimenB mind sending me an email to cpojer@fb.com so I get your email address, I'd like to invite you to our #jest-core channel. You've been doing stellar work on this project and we appreciate it a lot.

[SimenB]

One thought: can we somehow verify that we always apply this optimization? I would like us to codemod all of Jest to imports at some point and use the inline-imports transform then but we also need to make sure that once we do that, we aren't breaking this and slowing Jest down a lot.

That's what I'm doing here; converting from require to import and adding the babel-transform. The only requires left are ones importing variable names. At least that's the idea 😄
Could add https://github.com/benmosher/eslint-plugin-import/blob/master/docs/rules/no-commonjs.md to avoid regressions on it?

Might want to convert to export as well, but that's a breaking change (because of the added .default for programmatic usage). Can hold off on that until 21 is about to hit, then do the modules.exports => export conversion as well?

[SimenB]

Ok, this is ready for review now.

From @cpojer's tip, I've left requires alone in tests, only changing source code.

2 outstanding issues:

1. I added the allowTopLevelThis option to the babel-plugin, otherwise this throws. Might not be an issue, not sure?
2. 2 failing tests

   1. packages/jest-snapshot/src/__tests__/utils-test.js
      ● serialize handles \r\n
      

   2. integration_tests/__tests__/symbol-test.js
      ● Symbol deletion
      

The first failing test seems to be because we do json.parse(undefined), because pretty-format/.babelrc doesn't exist. Why it looks for this, I have no idea.
The second one I don't get at all...

[SimenB]

This is rebased now. Same two failures, which is interesting...

Summary of all failing tests
 FAIL  integration_tests/__tests__/symbol-test.js
  ● Symbol deletion

    TypeError: Cannot read property 'prototype' of undefined

      at Object.<anonymous> (packages/jest-jasmine2/node_modules/jest-matchers/node_modules/jest-matcher-utils/node_modules/pretty-format/build/index.js:54:30)

 FAIL  packages/jest-snapshot/src/__tests__/utils-test.js
  ● serialize handles \r\n

    SyntaxError: /Users/simbekkh/repos/jest/packages/pretty-format/package.json: Error while parsing JSON - Unexpected token u in JSON at position 0
        at Object.parse (native)

      at ConfigChainBuilder.addConfig (packages/babel-jest/node_modules/babel-core/lib/transformation/file/options/build-config-chain.js:150:65)

[cpojer]

Do you mind changing "transpiled" to "compiled" here and everywhere else?

[SimenB]

Sure!

[SimenB]

Did a yarn clean-all && yarn && yarn jest and we're down to one failure.

 FAIL  integration_tests/__tests__/symbol-test.js
  ● Symbol deletion

    TypeError: Cannot read property 'prototype' of undefined

      at Object.<anonymous> (packages/jest-jasmine2/node_modules/jest-matchers/node_modules/jest-matcher-utils/node_modules/pretty-format/build/index.js:54:30)

[cpojer]

Ok, I don't wanna be silly but could you do one more split?

• One PR with all the changes to imports/requires.
• One tiny PR that flips on the inline imports which we'll use to actually fix up the tests.

That way the major change you are making is reduced to a very small PR that can be reverted more easily.

[SimenB]

Ok, I don't wanna be silly but could you do one more split?

Done: #3780

[cpojer]

Somehow I couldn't push to your branch so I opened a new PR at #3786.

[github-actions]

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.