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

Update babel getter to work with babel-jest@24 #960

Closed
wants to merge 2 commits into from

Conversation

Jessidhia
Copy link

@Jessidhia Jessidhia commented Jan 28, 2019

babel-jest@24 will also use loadPartialConfig to be able to mix its own options into the config in an API-conforming manner. However, loadPartialConfig will throw an assertion error if the config has not only been loaded, but also had its plugins instantiatied. loadOptions() does the whole thing, including instantiating the plugins, so it can't be used for multiple config loading passes, only the final pass.

Verified by monkey-patching the local ts-jest install; looks like this:

            var _a = importer_1.importer.babelCore(messages_1.ImportReasons.BabelJest), OptionManager = _a.OptionManager, loadOptions = _a.loadPartialConfig, version = _a.version;
            if (version && semver_1.default.satisfies(version, '>=6 <7')) {
                delete base.cwd;
            }
            var config;
            if (typeof loadOptions === 'function') {
                console.warn('loadOptions', base)
                config = loadOptions(base).options;
            }

HOWEVER, this will just load a top level babelrc and stop babel-jest from searching for more config files because the partial config includes babelrc: false and configFile: false. The babel getter is not aware of which filename is going to be processed so this will likely break unless everything shares the same babel config and the babel config is in the process.cwd().

This is more of a starting point instead of a full fix.

babel-jest@24 will also use loadPartialConfig to be able to mix its own options into the config in an API-conforming manner. However, loadPartialConfig will throw an assertion error if the config has not only been loaded, but also had its plugins instantiatied. loadOptions() does the whole thing, including instantiating the plugins, so it can't be used for multiple config loading passes, only the final pass.
config = loadOptions(base) as BabelConfig
if (typeof loadPartialConfig === 'function') {
const partialConfig = loadPartialConfig(base)
config = partialConfig && partialConfig.options as BabelConfig

Choose a reason for hiding this comment

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

typeof partialConfig is Readonly<PartialConfig> | null so that cannot insert to config, error message report.

@artola artola mentioned this pull request Feb 15, 2019
@SimenB
Copy link
Contributor

SimenB commented Feb 15, 2019

As mentioned in the OP, ts-jest cannot assume that there's only a single babel config to be loaded for the whole project - it needs to load for every file (babel itself should take care of caching issues surrounding this). And since it sets configFile: false, it will short-circuit when jest calls loadPartialConfig later, so there's no risk of wasted fs operations (in theory) after being passed the result of this call

@Jessidhia
Copy link
Author

I wonder if ts-jest even needs to do the config loading at all; as long as it calls the babel-jest helpers for both the processing and caching it should work. Even if the user wants to have their own overrides via the ts-jest config it can just be passed as an argument to babel-jest.

@artola
Copy link

artola commented Feb 21, 2019

I wonder if ts-jest even needs to do the config loading at all; as long as it calls the babel-jest helpers for both the processing and caching it should work. Even if the user wants to have their own overrides via the ts-jest config it can just be passed as an argument to babel-jest.

I am of the same opinion, it would be nice if @kulshekhar give us some hint regarding this topic.

@kulshekhar
Copy link
Owner

I wonder if ts-jest even needs to do the config loading at all; as long as it calls the babel-jest helpers for both the processing and caching it should work. Even if the user wants to have their own overrides via the ts-jest config it can just be passed as an argument to babel-jest.

I am of the same opinion, it would be nice if @kulshekhar give us some hint regarding this topic.

@artola to be very honest, after the last big rewrite (which improved ts-jest a lot, thanks to @huafu) I'm a lot less familiar with the codebase. That means that when I have time, I can try and fix/improve things. However, it'll be hard for me to give any kind of hint or suggestion off hand. I mean I could but without a lot of confidence, there's a good chance it might end up wasting your time - which I really wouldn't want to do.

That said, if you have something that's working and passes the tests (even if it's something that's testing out a hypothesis without 100% knowledge), I'll be happy to merge it in as a PR.

@toolness
Copy link
Contributor

Hello! If you just need the TravisCI build to work before merging this PR, I've created #1004 to do that (alternatively, @Jessidhia can just cherry-pick 67e21c3 into this PR). My patch doesn't address the comments made in #960 (comment), as I don't understand enough about Babel to make that change.

GeeWee pushed a commit that referenced this pull request Mar 1, 2019
This is the same thing as #960 but with the type errors fixed, maybe.

If it's desirable I could make this a PR against `jessidhia/patch-1` instead of against `master`.

I am getting some really weird feedback when I run `npm test` though, so I am just starting this PR to see what Travis thinks of it.
@GeeWee
Copy link
Collaborator

GeeWee commented Mar 1, 2019

I've merged in the updated typings in #1004

@Palid
Copy link

Palid commented Mar 19, 2019

Hey, do you need help with that? I'd love to have this one merged in so we can upgrade jest to the newer version. If needed I can be of help. :)

@GeeWee
Copy link
Collaborator

GeeWee commented Mar 19, 2019

Would love some help with this. I don't think anyone has a clear idea what's left to do at the moment though, so there's very little guidance from our side.

@toolness
Copy link
Contributor

Er, so this actually already is merged, because it was included in #1004... see #1004 (comment) for more of the conversation.

Sorry about all this confusion. I wanted #1004 to be a PR against this PR, but doing that would've meant it'd show up in @Jessidhia's fork of the repo, not here, and I wasn't sure if she was paying attention to this PR anymore, so I put the PR here against master for more visibility, but then confusion ensued...

@Palid
Copy link

Palid commented Mar 19, 2019

@toolness could we then do a quick npm release with this? This works. Previous one doesn't really due to this error. 😢

@toolness
Copy link
Contributor

I'm fine with that, but I'm just a drive-by contributor :) For what it's worth I've also been using the version of the code with #1004 in it, via the following entry in my package.json, and it has been working swimmingly:

"ts-jest": "https://github.com/kulshekhar/ts-jest#670503f65a79ec62712e5e4488b734c092b20a0f"

But yes, a formal npm release would be great!

@kulshekhar
Copy link
Owner

I'll publish a new release after work today

@moimael
Copy link

moimael commented Mar 28, 2019

Any updates ?

@kulshekhar
Copy link
Owner

Sorry for the delay. A new version has now been published

@ahnpnl
Copy link
Collaborator

ahnpnl commented Jan 31, 2020

As mentioned in the OP, ts-jest cannot assume that there's only a single babel config to be loaded for the whole project - it needs to load for every file (babel itself should take care of caching issues surrounding this). And since it sets configFile: false, it will short-circuit when jest calls loadPartialConfig later, so there's no risk of wasted fs operations (in theory) after being passed the result of this call

hi @SimenB , when I call createTransformer from babel-jest, does babel-jest handles loading all necessary babel config for the whole project ? The reason I ask this question is because of suggestion from @Jessidhia and also comment from #1341. In my opinion, this is the best. ts-jest just collects whatever it can and pass the object to babel-jest without invoking babel to load config like current logic, the rest babel-jest will handle it.

@ahnpnl
Copy link
Collaborator

ahnpnl commented Feb 5, 2020

I will close this as now we let Babel-jest handle getting Babel config

@ahnpnl ahnpnl closed this Feb 5, 2020
@SimenB
Copy link
Contributor

SimenB commented Feb 5, 2020

Sorry, I missed your message @ahnpnl. Yeah, unless you wanna mutate the user's Babel config yourself, just delegating to babel-jest should work out 👍

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

Successfully merging this pull request may close these issues.

10 participants