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

Fix ability to transform dependencies required from globalSetup script #8143

Merged
merged 1 commit into from
Mar 20, 2019

Conversation

Volune
Copy link
Contributor

@Volune Volune commented Mar 18, 2019

Summary

Jest failed to run globalSetup script if it loads esm dependencies from node_modules, even with the transformIgnorePatterns configuration. (For example transformIgnorePatterns: [ 'node_modules/(?!(?:lodash-es)/)' ])
This is caused by pirates ignoring node_modules by default.
This PR disables this behavior, which is redundant with transformIgnorePatterns.

Test plan

Added a test in globalSetup.test.js

@Volune Volune changed the title Fix ability to transform specific node_modules dependencies required in global setup script Fix ability to transform dependencies required from globalSetup script Mar 18, 2019
@Volune Volune force-pushed the fix-global-setup-node-modules branch from 1c51f05 to d9f9018 Compare March 18, 2019 06:50
@SimenB
Copy link
Member

SimenB commented Mar 18, 2019

Thanks for the PR!

I thought about doing this, but decided against it (quick mention in #7562). While the default transformIgnorePatterns ignores node_modules, I was afraid of messing up people's test runs if they had customized it. But maybe that's not really an issue?

/cc @thymikee @jeysal wdyt?

@jeysal
Copy link
Contributor

jeysal commented Mar 18, 2019

If someone removes node_modules from the transformIgnorePatterns, there's probably a reason for it so I think in general we should respect that. If we add a helpful custom error message in case the transform fails and ship this in a major, I think it's fine.

@thymikee thymikee added this to the Jest 25 milestone Mar 18, 2019
@Volune Volune force-pushed the fix-global-setup-node-modules branch 3 times, most recently from 3037156 to 579c5b1 Compare March 19, 2019 04:17
@Volune
Copy link
Contributor Author

Volune commented Mar 19, 2019

I made a few changes following comments and after looking at #7562

  • I use a fake node_module instead of the lodash-es dependency (so don't need to run yarn and eslint is happy)
  • I disable the transformer (matcher will return false) if the transformer is already running. This should prevent issues with transformIgnorePatterns: []. I updated the test accordingly.

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

I like how this turned out, thanks!

CHANGELOG.md Show resolved Hide resolved
@SimenB
Copy link
Member

SimenB commented Mar 19, 2019

Could you rebase on master and fix the linting error?

@Volune Volune force-pushed the fix-global-setup-node-modules branch from 579c5b1 to d5eb488 Compare March 20, 2019 10:54
@codecov-io
Copy link

Codecov Report

Merging #8143 into master will decrease coverage by 0.04%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8143      +/-   ##
==========================================
- Coverage   62.28%   62.24%   -0.05%     
==========================================
  Files         265      265              
  Lines       10440    10448       +8     
  Branches     2538     2540       +2     
==========================================
  Hits         6503     6503              
- Misses       3352     3360       +8     
  Partials      585      585
Impacted Files Coverage Δ
packages/jest-core/src/runGlobalHook.ts 0% <0%> (ø) ⬆️

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 c5fd7aa...d5eb488. Read the comment docs.

@SimenB
Copy link
Member

SimenB commented Mar 20, 2019

Thanks!

@SimenB SimenB merged commit ecf7636 into jestjs:master Mar 20, 2019
@pedrottimark
Copy link
Contributor

For your info, I just yarn clean-all and it deleted e2e/global-setup-node-modules/node_modules/example/index.js file, which caused yarn test to fail:

/Users/mark/gitforks/jest/e2e/global-setup-node-modules/setup.js
  7:21  error  Unable to resolve path to module './node_modules/example'  import/no-unresolved

@SimenB
Copy link
Member

SimenB commented Mar 20, 2019

Bah. @Volune wanna send a PR fixing our script in package.json?

@Volune
Copy link
Contributor Author

Volune commented Mar 20, 2019

I can have a look tomorrow (I don't have my dev environment now)

Suggestions welcome, I'm thinking:

  • put back lodash-es as a dependency (with eslint-ignore for the unresolved import)
  • copy the file to node_modules in the test (also with eslint-ignore) (favoring this one for the moment)
  • change the clean-e2e script

@pedrottimark
Copy link
Contributor

pedrottimark commented Mar 20, 2019

@Volune @SimenB I will open PR to add another exception to clean-e2e for this test: #8174

@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 11, 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.

7 participants