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 type errors in jessidhia/patch-1 #1004

Merged
merged 4 commits into from
Mar 1, 2019

Conversation

toolness
Copy link
Contributor

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.

Jessidhia and others added 4 commits January 29, 2019 00:17
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.
@coveralls
Copy link

coveralls commented Feb 28, 2019

Pull Request Test Coverage Report for Build 2439

  • 5 of 7 (71.43%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.02%) to 91.933%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/config/config-set.ts 5 7 71.43%
Totals Coverage Status
Change from base Build 2437: 0.02%
Covered Lines: 913
Relevant Lines: 958

💛 - Coveralls

@toolness
Copy link
Contributor Author

Oh neat, the tests appear to have passed.

Note that this PR doesn't address #960 (comment) (unfortunately I don't understand enough about Babel to address that), it just fixes typings so the PR doesn't break the build.

Copy link
Collaborator

@GeeWee GeeWee left a comment

Choose a reason for hiding this comment

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

I'll be honest, I'm not entirely sure if this is the right way to do it, but it's a step in the right direction so let's see where it goes!

@GeeWee GeeWee merged commit 670503f into kulshekhar:master Mar 1, 2019
@toolness
Copy link
Contributor Author

toolness commented Mar 1, 2019

Thanks @GeeWee! Since you merged this into master, should #960 be closed?

Also, how soon do you think you will publish a new release to npm? I originally tried setting my package.json's entry for ts-jest to https://github.com/kulshekhar/ts-jest#670503f65a79ec62712e5e4488b734c092b20a0f but that doesn't seem to work because it looks like this project has a build step that runs before publishing to npm...

@toolness
Copy link
Contributor Author

toolness commented Mar 1, 2019

Ack, ignore the part about re-publishing to npm--I realized ts-jest has a npm prepare script and that it wasn't executing on my CI system for a funky reason. Just fixed that and I'm able to use 670503f directly now!

@GeeWee
Copy link
Collaborator

GeeWee commented Mar 2, 2019

Why should #960 be closed? Isn't it about more than the typings?

@toolness
Copy link
Contributor Author

toolness commented Mar 2, 2019

Er yes, #960 is about more than the typings, but it contains broken typings... This PR was meant to fix #960's broken typings, and as such it builds upon #960 (note that this PR's first two commits are ebe8237 and
3c2b1cf, the same two commits that constitute #960)... 😣

@GeeWee
Copy link
Collaborator

GeeWee commented Mar 8, 2019

Aw dang, I actually thought this was just a fix that allowed #960 to move on. I shouldn't have merged this in. Oh well.

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.

4 participants