Document Asynchronous Configurations in the Config File's Docs #3733
Conversation
npetruzzelli
changed the title
|
The default behavior of the server doesn't support promises, so some additional work will need to be done on the users side to get it working. I need additional time to add that to this PR. The changing the default behavior to support promises would be a breaking change. |
npetruzzelli
changed the title
|
I looked, but didn't see a way to compile/serve the docs locally (maybe I missed something obvious?) so I'm not sure if I linked to from the config file page to the public api page correctly. Otherwise this should be ready for review! |
|
@npetruzzelli The docs infra is located in the https://github.com/karma-runner/karma-runner.github.com repo. It's probably the easiest to copy your changes over to that repo and run |
|
Updated. I intentionally left out arrow functions and async/await. My thinking being that I want the focus to be on the use of promises. An experienced developer will already know to use those things (async/await) to simplify code, while a newer developer may be more anxious when seeing more unfamiliar things when they aren't required. Is that acceptable? |
|
@npetruzzelli Can you just squash into a single commit and remove the trailing "." from the commit message to make CI happy? |
|
I don't often manually squash or rebase, so I'm not sure I did it correctly. |
|
@npetruzzelli No, does not look right, but no worries, I think @jginsburgn can squash when merging. I usually can fix such issues myself, thanks to "Allow edits by maintainers" checkbox, but it does not seem to work in this case, because your fork is in the organisation. Or maybe I do something wrong. I think we can let it be for now. |
There was a problem hiding this comment.
@npetruzzelli please excuse my long delay in getting to this.
Can you please rebase and squash? If you don't remember how to squash, maybe you can give me push permissions to your fork?
docs/config/01-configuration-file.md
Outdated
| } | ||
| ``` | ||
|
|
||
| If you are using the `parseConfig`method from the public API, then please see |
There was a problem hiding this comment.
Nit: missing space after parseConfig.
There was a problem hiding this comment.
@jginsburgn - I will try to fix the typo and rebase/squash by this weekend.
docs/config/01-configuration-file.md
Outdated
|
|
||
| module.exports = function configKarma(config) { | ||
| const myPreferredPort = 9876; | ||
| return findAvailablePort(myPreferredPort) |
There was a problem hiding this comment.
This is a bad example IMHO, because it leaves the door wide open for race conditions to occur.
There was a problem hiding this comment.
The most common (simplified) description of a race that I've had experience with is that multiple operations are being executed in parallel with no way to control the order of execution, which can lead to an unpredictable/unstable state.
I don't see the potential for race conditions here. The hypothetical findAvailablePort returns a promise. That promise must resolve before the onFulfilled parameter executes (which then executes config.set).
As long as it is a Promise instance, or an object that is a conforming implementation of Promises/A+, then the order of operations is controlled and there is no race.
Are you concerned about promise rejections not getting handled in the example?
There was a problem hiding this comment.
I mean that getting a free port and then using that port. The port can be claimed by another process in the mean time.
I've experienced this twice on the past.
- On an old fashion buildserver that ran parallel builds.
- During mutation testing with StrykerJS, where we also need to spin up multiple test runner processes.
The right way to choose a free port is by leaving it undefined and letting karma itself choose it.
There was a problem hiding this comment.
That is reasonable. In the time it takes between getting a port number and starting the server, something else could claim the port, especially if other async tasks happen before then.
The example can easily be changed to another process that is often asynchronous, "getThingFromFileSystem" or "getThingFromNetwork". I would rather the example draw attention to "hey, this is asynchronous" instead of "this is a way to solve this specific problem/task" even if the intent was for illustration. I'd even be ok with the super generic "doAsyncThing", as long as it gets the point across that it returns a promise.
There was a problem hiding this comment.
Exactly. I didn't see this PR at first and opened #3761 where I add documentation like that.
npetruzzelli
force-pushed
the
config-file-docs
branch
from
d8b5c0e to
b8eafe9
Compare
|
I managed to make a mistake and accidentally closed this PR as a side effect of hard resetting my branch. The new PR can be found here: #3762 |
See: #3677 (comment)