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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

babel 7 with dynamic requires does not work #7438

Closed
chrisblossom opened this issue Nov 29, 2018 · 18 comments 路 Fixed by #7574
Closed

babel 7 with dynamic requires does not work #7438

chrisblossom opened this issue Nov 29, 2018 · 18 comments 路 Fixed by #7574

Comments

@chrisblossom
Copy link
Contributor

chrisblossom commented Nov 29, 2018

馃悰 Bug Report

When using Babel 7, Jest does not transform dynamic required files. Babel 6 works as expected.

This is a continuation of #7021.

Link to repl or repo (highly encouraged)

chrisblossom/jest-issue-7438

Run npx envinfo --preset jest

Paste the results here:

  System:
    OS: macOS 10.14.1
    CPU: (16) x64 Intel(R) Xeon(R) W-2140B CPU @ 3.20GHz
  Binaries:
    Node: 10.13.0 - ~/.nvm/versions/node/v10.13.0/bin/node
    npm: 6.4.1 - ~/.nvm/versions/node/v10.13.0/bin/npm
  npmPackages:
    jest: 23.6.0 => 23.6.0
@thymikee
Copy link
Collaborator

You also need to install babel-jest explicitly, can you check if it changes anything?

@42tg
Copy link

42tg commented Nov 30, 2018

You have to add an custom transformer to solve this Babel 7 issues... I have added a Pull request to your repository wich solves it

@chrisblossom
Copy link
Contributor Author

chrisblossom commented Jan 3, 2019

@thymikee adding babel-jest did not solve the issue, but the pull request that @42tg submitted did.

@SimenB
Copy link
Member

SimenB commented Jan 3, 2019

We use babel itself to lookup config in Jest 24, so this has been solved on master.

Install jest@beta (currently jest@24.0.0-alpha.9) to use it today

@SimenB SimenB closed this as completed Jan 3, 2019
@chrisblossom
Copy link
Contributor Author

@SimenB jest@24.0.0-alpha.9 does not appear to fix the issue. chrisblossom/jest-issue-7438/tree/jest-24

@SimenB
Copy link
Member

SimenB commented Jan 3, 2019

That's what I get for not actually testing 馃槄


First of all, you also need to update babel-jest (and you can drop babel-core)

diff --git i/package.json w/package.json
index e61a4c6..978b95b 100644
--- i/package.json
+++ w/package.json
@@ -12,8 +12,7 @@
   "devDependencies": {
     "@babel/core": "7.2.2",
     "@babel/preset-env": "7.2.3",
-    "babel-core": "7.0.0-bridge.0",
-    "babel-jest": "23.6.0",
+    "babel-jest": "24.0.0-alpha.9",
     "jest": "24.0.0-alpha.9"
   }
 }

However, that does not solve your issue. The issue is due to the fact you do process.chdir, which causes babel to not find you configuration (since there's no longer a babel config file in cwd). If you want to change directory, then you need to tell babel where to find your config. The reason it works if you do not lazy import, is that we are able to transform before you change directory, and babel is able to find your config.

(A case might be made that Jest should not give you access to the real process.chdir, but then we'd have to fake process.cwd() for you... and it's a rabbit hole.)

A potential solution (other than the PR you received) is to tell babel to use rootMode: 'upward' (https://babeljs.io/docs/en/options#rootmode) and rename .babelrc.js to babel.config.js.


Another thing that fixes it is setting root to config.rootDir (https://babeljs.io/docs/en/options#root) instead of the default process.cwd(). However, that's something Jest has to potentially do here: https://github.com/facebook/jest/blob/103f0685f90495df3b0e0a06b23ab182b6aea4e4/packages/babel-jest/src/index.js#L38-L46, it's not something you can set from the outside (or, you can set root: process.cwd() in babel-jest's config, but that won't work for multiple projects)

@loganfsmyth do you think Jest should be setting root to config.rootDir (https://jestjs.io/docs/en/configuration#rootdir-string) when calling loadPartialConfig?


Note that we have a PR allowing you to configure the transformer (#7288) which should be slightly less chunky than having to call createTransformer

@SimenB SimenB reopened this Jan 3, 2019
@loganfsmyth
Copy link

Babel accepts a cwd option that it uses as the default for path processing, so if Jest saves calculates that once up front, which seems ideal.
I don't think I'd set root because it's really more dependent on user project layout, so it's something I'd probably expect the user to set. If that's getting easier, that's excellent too though.

@SimenB
Copy link
Member

SimenB commented Jan 4, 2019

Ah, that makes more sense, yeah 馃檪

However, rootDir might not be in the actual root of the project - it's just wherever the Jest config was found (although a user can specify rootDir to be wherever they want within that config). So it's not necessarily the directory containing babel config (root or otherwise) or even in the parent path of test files (imagine <rootOfProject>/configs/jest.config.js). Do you still think it makes sense to set it? I'm a bit afraid it might potentially cause hard-to-debug errors for users (though I've mostly seen people setting rootConfig if the jest config is not in root, otherwise finding tests and modules are a bit finicky, so maybe not an issue)

@loganfsmyth
Copy link

Oh I should have clarified, my suggestion was to call process.cwd() once when Jest starts and pass that to Babel, not that rootDir should be passed as cwd to Babel.

@SimenB
Copy link
Member

SimenB commented Jan 4, 2019

Right! Yeah, we can do that.

@chrisblossom
Copy link
Contributor Author

@SimenB Thanks for reopening the issue and looking further into it.

(A case might be made that Jest should not give you access to the real process.chdir, but then we'd have to fake process.cwd() for you... and it's a rabbit hole.)

I'd personally vote against this. I use process.chdir quite often with my tests. I initially tried mocking process.cwd, but that caused several edge-case issues (for example, mocking process.cwd doesn't update path.resolve) and at the end of the day does not work.

I don't mind having to use a custom transformer just as long as I can continue to change process.chdir as I do currently.

Babel accepts a cwd option that it uses as the default for path processing, so if Jest saves calculates that once up front, which seems ideal.

This sounds like the best solution to me if it works out of the box.

@SimenB
Copy link
Member

SimenB commented Jan 4, 2019

See #7574

@chrisblossom
Copy link
Contributor Author

I just wanted to confirm that this issue has been fixed with jest@24.0.0-alpha.12. Thank you!!

@kopax
Copy link

kopax commented Jan 31, 2019

Is alpha.12 before 24.0.0 ? I have upgrade to 24.0.0 and I still have the issue. I am using babel.config.js :

module.exports = {
  only: [
    'src',
    'styleguide',
  ],
  presets: [
    [
      '@babel/preset-env',
      {
        modules: false,
      },
    ],
    '@babel/preset-react',
  ],
  plugins: [
    'babel-plugin-styled-components',
    'babel-plugin-array-includes',
    '@babel/plugin-syntax-dynamic-import',
    '@babel/plugin-syntax-import-meta',
    '@babel/plugin-proposal-class-properties',
    '@babel/plugin-proposal-json-strings',
    [
      '@babel/plugin-proposal-decorators',
      {
        legacy: true,
      },
    ],
  ],
  env: {
    production: {
      plugins: [
        'add-module-exports',
        '@babel/plugin-transform-modules-commonjs',
      ],
    },
    test: {
      plugins: [
        '@babel/plugin-transform-modules-commonjs',
        'dynamic-import-node',
      ],
    },
  },
};

@SimenB
Copy link
Member

SimenB commented Jan 31, 2019

@kopax yeah, 24.0.0 is a stable release made after all the alphas. Could you put together a repository we can pull down that reproduces the issue?

@kopax
Copy link

kopax commented Jan 31, 2019

git clone git@github.com:bootstrap-styled/v4.git
cd v4
git checkout dev-jest24
npm i 
npm test

There's also a detailled issue on stackoverflow
Travis build failure: https://travis-ci.org/bootstrap-styled/v4/jobs/486846842#L614

@SimenB
Copy link
Member

SimenB commented Jan 31, 2019

Thanks, let's continue in #7765

@github-actions
Copy link

This issue 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.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants