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

await for async jest config #8357

Closed
wants to merge 4 commits into from
Closed

await for async jest config #8357

wants to merge 4 commits into from

Conversation

hisco
Copy link
Contributor

@hisco hisco commented Apr 21, 2019

Summary

Currently jest cli require the dynamic config module synchronously, this PR enables loading jest config module async.

Test plan

Tested project using yarn test - got some issues not related to my change:

/jest/packages/jest-haste-map/src/lib/FSEventsWatcher.ts
14:1  error  Unused eslint-disable directive (no problems were reported)
/jest/packages/jest-transform/src/ScriptTransformer.ts
578:3  warning  Arrow function expected no return value  consistent-return
594:3  warning  Arrow function expected no return value  consistent-return

In addition created a tiny project and tested the async config module using yarn link - no issues were observed.

@scotthovestadt
Copy link
Contributor

You can run tests with yarn jest, looks like there are a few failing.

Can you give me an example of how this is used and also update documentation with that as well?

@hisco
Copy link
Contributor Author

hisco commented Apr 24, 2019

@scotthovestadt thanks

  • I run yarn test the falling tests indeed show up.
  • Updated the contribution section.
  • Updated the configuration section.

I added an example of such use is in the docs section, here a more advanced section:

import * as request from 'request-promise';
export async ()=>{
  return await request('http://some-remote')
}

Basically for my use cases, I found a way to do it synchronically, I think it would be much nicer to do it async.

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.

Please add a test (integration test perhaps?) with async config. Also please update the changelog and fix the linting error reported by CI 🙂


This is technically a breaking change if anybody else uses jest-config to load jest's config instead of letting Jest itself do it. Is that ok?

@jeysal
Copy link
Contributor

jeysal commented Apr 24, 2019

+1 for breaking, put it in a major 😅

@hisco
Copy link
Contributor Author

hisco commented Apr 26, 2019

@SimenB I didn't notice any integration test for current implementation, do you have an idea how to mock this scenario?

Thanks in advance!

@SimenB
Copy link
Member

SimenB commented Nov 9, 2019

This is a really small breaking change, so we can land it in 25. @hisco could you rebase on master?

@hisco
Copy link
Contributor Author

hisco commented Nov 19, 2019

@SimenB I did, unfortunately it's completely broken:
Latest commit broke it (6bd4e560e288b515ebdf8325f6219522f538dbec)
At commit 8d0b5e2234d378920eb87112933c5c03016fb98e everything was working properly.

@SimenB
Copy link
Member

SimenB commented Feb 13, 2020

@hisco sorry, I missed your answer, then completely forgot about this 😬

I made readConfig async as part of support jest.config.mjs in #9431, so most of the changes in this PR are already on master (and it is no longer breaking as the breaking changes are already landed). It should be a matter of invoking the function after loading in packages/jest-config/src/readConfigFileAndSetRootDir.ts and adding a test plus updating docs. Are you up for finishing this, by any chance?

@SimenB
Copy link
Member

SimenB commented Apr 23, 2020

@hisco ping 🙂

@hisco
Copy link
Contributor Author

hisco commented May 7, 2020

It seems that most of my changes are already in master I don't think it's a result of a merge with my code so I won't take the credit...
The only thing missing was the docs, please use #10001 instead it's a much cleaner branch.

@hisco hisco closed this May 9, 2020
@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.

5 participants