-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
feat: support asynchronous config.set()
call in karma.conf.js
#3660
feat: support asynchronous config.set()
call in karma.conf.js
#3660
Conversation
✅ Build karma 540 completed (commit eeaff6b835 by @npetruzzelli) |
✅ Build karma 2937 completed (commit eeaff6b835 by @npetruzzelli) |
@devoto13 - I'm not sure if I will be able to see this PR through to completion (quickly), but I was able to at least start the work so that the conversation/review can start. I can't help but to "throw my hat into the ring" as it were. I initially opened #3631 / #3635 because I was working on a personal module that would support ES Modules and Promises. It still needs a lot of work, including the addition of tests (which is why it is only v |
✅ Build karma 539 completed (commit eeaff6b835 by @npetruzzelli) |
Would this need the ability for users to pass their own promise implementation, or will the default Promise meet the needs of all environments we expect this to serve in? |
✅ Build karma 541 completed (commit 527337b0be by @npetruzzelli) |
✅ Build karma 2938 completed (commit 527337b0be by @npetruzzelli) |
✅ Build karma 540 completed (commit 527337b0be by @npetruzzelli) |
✅ Build karma 2939 completed (commit ff32cf5f74 by @npetruzzelli) |
✅ Build karma 541 completed (commit ff32cf5f74 by @npetruzzelli) |
ASIDE: Should the |
It still needs new tests to be written, but I am marking this as ready for review to address feedback prior to putting time into tests so that I can avoid writing tests that may be made obsolete by review feedback. |
parseConfig
parseConfig
parseConfig
parseConfig
✅ Build karma 542 completed (commit ff32cf5f74 by @npetruzzelli) |
No worries, take your time. And thanks for the work you're putting into implementing this!
Default implementation is enough.
I'm not sure how useful is this. Better have strong real use case for this and discuss it before doing anything.
I'll take a look right away. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks great @npetruzzelli! Thanks a lot for doing this.
I left a couple of tiny comments. Let me know if they make sense to you.
I've also checked point (4). Looking at the PR code, I don't think we need to do anything about it.
✅ Build karma 546 completed (commit b08ab63426 by @npetruzzelli) |
✅ Build karma 2943 completed (commit b08ab63426 by @npetruzzelli) |
✅ Build karma 545 completed (commit b08ab63426 by @npetruzzelli) |
✅ Build karma 561 completed (commit cbdb405e59 by @npetruzzelli) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good! Left couple of comments to cleanup.
✅ Build karma 565 completed (commit 936aa92bba by @npetruzzelli) |
✅ Build karma 2962 completed (commit 936aa92bba by @npetruzzelli) |
✅ Build karma 564 completed (commit 936aa92bba by @npetruzzelli) |
✅ Build karma 565 completed (commit 65f3b8fbbd by @npetruzzelli) |
✅ Build karma 566 completed (commit 65f3b8fbbd by @npetruzzelli) |
@devoto13 - While working on updates for this round of review, I was running into an issue where random tests in
I haven't been able to replicate the problem as of the latest commit. It may not be an issue anymore, but I wanted to make you aware of it. |
parseConfig
config.set()
call in karma.conf.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the title and description to provide some useful information to future readers. It looks good to go now.
@npetruzzelli Thanks for doing all the hard work! 💪
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome, thanks
# [6.3.0](v6.2.0...v6.3.0) (2021-03-23) ### Features * support asynchronous `config.set()` call in karma.conf.js ([#3660](#3660)) ([4c9097a](4c9097a))
🎉 This PR is included in version 6.3.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
You're most welcome! Glad to help! |
…a-runner#3660) The existing sync behavior co-exists with the new async behavior. * add promise support to `parseConfig` * add async config support to `cli`, `runner`, and `stopper` * Additional API for `Server()` accepting parsed config. Older API is deprecated. * update documentation for parseConfig * add warning for deprecated use of CLI options * update Server constructor, runner, and stopper docs
# [6.3.0](karma-runner/karma@v6.2.0...v6.3.0) (2021-03-23) ### Features * support asynchronous `config.set()` call in karma.conf.js ([karma-runner#3660](karma-runner#3660)) ([4c9097a](karma-runner@4c9097a))
With this change, Karma allows to return a
Promise
from a configuration function directly or make such functionasync
and thus allows to perform some asynchronous initialization/fetch configuration from somewhere. When the configuration function returns a Promise, Karma will wait until the promise is resolved before it proceeds with the next steps.Example usage:
For Karma's end-users the change is fully backward compatible. For Karma's programmatic users the change is backward compatible, but the current behavior of
parseConfig()
,Server
,runner
, andstopper
is deprecated and will change in the next major release. New behavior (which supports asynchronous configuration function) can be enabled with a flag. Please refer to the public API documentation for more details.Fixes #1259