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

Different solution for babel-ignored files (#7765) #7800

Closed
leoselig opened this issue Feb 4, 2019 · 6 comments
Closed

Different solution for babel-ignored files (#7765) #7800

leoselig opened this issue Feb 4, 2019 · 6 comments

Comments

@leoselig
Copy link

leoselig commented Feb 4, 2019

I am referring to the bug already reported here #7765
More specifically, I'm questioning the solution just merged via #7797

Problem

I believe that printing the warning is not a user-friendly solution to this. My reasoning for that is that in a Babel/Jest setup I have two concerns:

  • I want to transpile my code using babel and use babel's configuration system to tell it what files to transpile how.
  • I want to write and run my tests based on Jest.

Jest shouldn't care about my babel configuration. I already define that in potentially numerous/complex .babelrc/js/overrides/env/ignore.

The change in the PR #7797 forces me to re-define a distilled (i.e. "effective") version of my include/eclude/ignore configuration for Jest.

Proposed solution

I believe that loadBabelConfig() returning null indicates that Babel decided to explicitly not transpile a file.


(I was trying to add this to the PR before the merge but was minutes too late. Let's see, maybe I'm mistaken here)

@SimenB
Copy link
Member

SimenB commented Feb 4, 2019

I think #7797 is still better than what is in 24.0.0, since you'll get an error either way - the difference is we now give you an actionable error.


That said, I do agree with your reasoning (duplicating config is bad). However, we currently implement skipping transforms via transformIgnorePattern - there's no way for a transformer to opt-out on its own. So we either have to add a new API to transformers or do weird stuff in getCacheKey.

This is sorta similar to webpack aliases - we require you to copy them to moduleNameMapper as we have no intention of attempting to extract this information from webpack's config. However, we have a way tighter integration with Babel (and we bundle babel-jest), so it might make sense to not be as strict with it.

@thymikee @jeysal @rubennorte thoughts here?

@thymikee
Copy link
Collaborator

thymikee commented Feb 4, 2019

We read quite a bunch of stuff from Babel already, so if there's a simple way to map only, ignore etc to transformIgnorePatterns that would be nice from our side. But it's one more thing to maintain. It would also open us up for further "integrations" like that and possibly making it even harder to upgrade to newer Babel versions (and we remember how painful migration to v7 was)

@rubennorte
Copy link
Contributor

That said, I do agree with your reasoning (duplicating config is bad). However, we currently implement skipping transforms via transformIgnorePattern - there's no way for a transformer to opt-out on its own. So we either have to add a new API to transformers or do weird stuff in getCacheKey.

We read the Babel configuration in both the getCacheKey and the process methods in the transformer, so I don't see anything that prevents us from ignoring a file for which the configuration is null (except when instrumentation is enabled, when we must use the istanbul plugin).

@rubennorte
Copy link
Contributor

@thymikee that's possible too, but we could do it in the process function anyway (if a file doesn't need to be transformed we can just return the original source). The only problem would be the opposite case, when transformIgnorePattern ignores a file and the transformer wouldn't.

@github-actions
Copy link

This issue is stale because it has been open for 1 year with no activity. Remove stale label or comment or this will be closed in 14 days.

@github-actions github-actions bot added the Stale label Feb 25, 2022
@github-actions
Copy link

This issue was closed because it has been stalled for 7 days with no activity. Please open a new issue if the issue is still relevant, linking to this one.

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

No branches or pull requests

4 participants