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

Gracefully return not found if jest config not found #32

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

outoftime
Copy link

Since the resolver looks up the Jest config relative to the file containing the import statement, it will not expect to find a configuration when recursing through package dependencies. This is fine, and just means that the resolver should return not found, allowing the import plugin to fall back to a regular node resolver.

Unfortunately this does make tracking down problems where the configuration actually is missing more difficult, but I don't see a way to fix that without completely overhauling the way configuration lookup is done.

Since the resolver looks up the Jest config relative to the file
containing the import statement, it will not expect to find a
configuration when recursing through package dependencies. This is fine,
and just means that the resolver should return not found, allowing the
import plugin to fall back to a regular node resolver.

Unfortunately this does make tracking down problems where the
configuration actually is missing more difficult, but I don't see a way
to fix that without completely overhauling the way configuration lookup
is done.
@chmanie
Copy link
Member

chmanie commented Aug 13, 2019

Hey @outoftime, thank you so much for your PR!

I have some questions here.

Unfortunately this does make tracking down problems where the configuration actually is missing more difficult

Can you elaborate on that a bit? IMHO findRoot should always find the root of the project regardless how deep you're in the dependency tree. If you don't have a jest config in your root, then this package is failing its point.

@outoftime
Copy link
Author

If you don't have a jest config in your root, then this package is failing its point.

Agreed! The case I’m concerned about is where the jest config is missing because of a user error (e.g. setting the wrong jestConfig property in the plug-in configuration). Currently, that situation will result in a pretty clear error message—but with this PR in place, it would just silently fall back to the default Node config.

@chmanie chmanie closed this Aug 20, 2019
@outoftime
Copy link
Author

Hi @chmanie, curious why you decided to close this without merging?

@chmanie
Copy link
Member

chmanie commented Aug 21, 2019

Hey, sorry, I really didn't mean to close it. I think must have misclicked.

Let's keep the discussion going. I'll get back to you when I had a bit more time to think about this.

@chmanie chmanie reopened this Aug 21, 2019
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants