Simplify promise queue implementation #102
Conversation
|
@booc0mtaco @timzchang @jeremiah-clothier looks like a maintainer needs to approve the CI workflow. Also, please review when you get a chance 🙏 |
Just to make sure promise rejections are handled.
I can't see the settings on this repo, I'm not sure how this is currently configured |
And into the AxePage code, with the rest of the axe-core logic.
Keep all Axe and playwright stuff in the "Browser" code.
| 'landmark-one-main', | ||
| 'page-has-heading-one', | ||
| 'region', | ||
| ]; |
There was a problem hiding this comment.
Moved the default disabled rules into the AxePage file, so that Axe stuff is more centralized there.
| story.config, | ||
| ); | ||
| return new Result(axeResults.violations); | ||
| } |
There was a problem hiding this comment.
Moved this function into TestBrowser, so that Playwright code is centralized there.
| return new Promise<T>((resolve, reject) => { | ||
| queue = queue.then(createPromise).then(resolve).catch(reject); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
A change with this new implementation is the it'll stop processing subsequent promises if one is rejected (unless we add rejection handing somewhere, which I might do at some point).
Regardless, I think this is fine, because:
- The
axe.runpromise will not normally be rejected (even if there are axe violations). So something has gone very wrong with the axe run if it is. - This whole queue mechanism is a fallback that might not even be necessary. It shouldn't be possible to end up with multiple
axe.run's at a time. This queue is probably overly paranoid.
| story.runOptions, | ||
| story.context, | ||
| story.config, | ||
| ); |
There was a problem hiding this comment.
As noted elsewhere, I moved this code here. That way the Result object doesn't need to contain any Playwright stuff.
|
@ahuth thanks for the refactor and contribution! will have a look this evening |
| return new Promise<T>((resolve, reject) => { | ||
| queue = queue.then(createPromise).then(resolve).catch(reject); |
There was a problem hiding this comment.
Am I understanding correctly that the first .then invokes the promise at the end of the queue, and then createPromise is invoked, adding on to the queue?
Does the second .then then resolve the one just created by createPromise?
Sorry a lot of promises going around 😅
There was a problem hiding this comment.
Sort of!
.then creates a new promise off of the current queue (which of course is then set as the current queue). It's resolved with the value the created promise is resolved with...
Might be more clear if I wrote it out as
queue = queue.then(() => {
return createPromise();
})of if we made it less generic and inlined the promise creation (for illustration purposes), it would look something like
// For each story
queue = queue.then(() => {
return axe.run(...);
})Going even deeper, if we unrolled the loop it would be like
Promise.resolve()
.then(() => axe.run(...)) // Story 1
.then(() => axe.run(...)) // Story 2
.then(() => axe.run(...)) // Story 3
// etc.Hope that helps!
There was a problem hiding this comment.
I guess the other way to say it is that .then creates a promise, and createPromise creates a different promise, but they are linked
|
Ok, thanks! If you run into any issues doing that, lemme know. There's a wiki article in this repo about publishing. |
This PR
axe.runcalls, which doesn't work), so it's easier to understand.