fix(notifier): For durable fail(), persist the reason rather than a rejected promise
#7011
Conversation
… rejected promise Fixes #7009
packages/notifier/src/publish-kit.js
Outdated
| if (targetStatus === 'failed') { | ||
| canBeDurable(value) || | ||
| Fail`Cannot accept non-durable fail reason: ${value}`; | ||
| } else if (done || valueDurability === 'mandatory') { | ||
| canBeDurable(value) || Fail`Cannot accept non-durable value: ${value}`; |
There was a problem hiding this comment.
I'm not sure it's worth keeping a distinct message for fail(nonDurableReason) vs. finish(nonDurableFinalValue) and publish(nonDurableValue). If not, we could simplify this.
| if (targetStatus === 'failed') { | |
| canBeDurable(value) || | |
| Fail`Cannot accept non-durable fail reason: ${value}`; | |
| } else if (done || valueDurability === 'mandatory') { | |
| canBeDurable(value) || Fail`Cannot accept non-durable value: ${value}`; | |
| if (done || valueDurability === 'mandatory') { | |
| canBeDurable(value) || Fail`Cannot accept non-durable value: ${value}`; |
There was a problem hiding this comment.
The stack trace should show which case it was. The only value I can think of is for a test to be able to verify which failure, but it could do that by exercising both paths. So +1 to the simplification.
packages/notifier/src/publish-kit.js
Outdated
| if (targetStatus === 'failed') { | ||
| canBeDurable(value) || | ||
| Fail`Cannot accept non-durable fail reason: ${value}`; | ||
| } else if (done || valueDurability === 'mandatory') { | ||
| canBeDurable(value) || Fail`Cannot accept non-durable value: ${value}`; |
There was a problem hiding this comment.
The stack trace should show which case it was. The only value I can think of is for a test to be able to verify which failure, but it could do that by exercising both paths. So +1 to the simplification.
packages/notifier/src/publish-kit.js
Outdated
| * @param {any} value | ||
| * @param {any} [rejection] | ||
| * @param {DurablePublishKitState["status"]} [targetStatus] |
There was a problem hiding this comment.
This is a type syntax I have not seen before. What's the difference between
DurablePublishKitState["status"]vs
DurablePublishKitState.statusor
DurablePublishKitState['status']?
There was a problem hiding this comment.
The two quotes are just stylistic. Our Prettier config is single quotes but it doesn't reach into jsdoc.
The . is reserved for referencing a namespace.
Fixes #7009
Description
Send the
failreason directly toadvanceDurablePublishKitso it can persist the value before wrapping it in a rejected promise to resolve the outstanding tail promise.Security Considerations
None.
Scaling Considerations
No change.
Documentation Considerations
Not applicable.
Testing Considerations
#6523 should make it possible to update the durable publish kit tests for realistic durability constraints.