NServiceBus
NServiceBus copied to clipboard
Particular
Saga.RequestTimeout API doesn't use DateTimeOffset
The sagas timeout API has the following signature: Task RequestTimeout<TTimeoutMessageType>(IMessageHandlerContext context, DateTime at)
it still uses DateTime although we switched the delayed delivery APIs on SendOptions to DateTimeOffset. Internally, the parameter will be converted to a DateTimeOffset as it's calling options.DoNotDeliverBefore(at);.
It's probably not huge deal, but at least from a consistency perspective we should align this API with the other delay delivery APIs and have it accept DateTimeOffset parameters.
Notice that although DateTime can be implicitly casted to DateTimeOffset this is still a binary breaking change.
@SimonCropp We are added a breaking changes for v8. DateTime is implicitly converted to DateTimeOffset and code would require a recompile anyway. Any objections on that? I think you are suggesting to have both an overload for DateTime and DateTimeOffset and for what benefit?
i have no problem with the breaking change in v8. but there are a couple of reasons to add the DateTimeOffset override in the next minor:
- you can obsolete-with-warn the DateTime one to let people know the change is coming
- you can deliver the DateTimeOffset version now
Also "DateTime is implicitly converted to DateTimeOffset and code would require a recompile anyway. " is a convenience that only words for the top level app. any extensions that use that api, and are consumed as dlls/nugets, will need to be deployed to prevent a missing method exception. adding DateTimeOffset now lets them move over now and avoid that immediate deployment requirement for that breaking scenario
reasons to add the DateTimeOffset override in the next minor:
@SimonCropp That would mean in the next v7 minor. Not sure if we will still add a minor but I'll forward this suggestion.
any extensions that use that api, and are consumed as dlls/nugets, will need to be deployed to prevent a missing method exception.
Wouldn't these target a specific major due to us following SemVer?
Are you maintaining extensions that have backward compatibility with multiple core majors?
@SimonCropp we are not planning a 7.5 release at the moment and we didn't think this change has enough value on it's own to release as 7.5. If we do decide to release a new minor, we will consider adding these overloads.
