sdk-go icon indicating copy to clipboard operation
sdk-go copied to clipboard

Switch to a new clock library

Open albertteoh opened this issue 2 years ago • 3 comments
trafficstars

Is your feature request related to a problem? Please describe. The Temporal go SDK uses the archived https://github.com/facebookarchive/clock.

Reference: https://github.com/temporalio/sdk-go/blob/master/go.mod#L6

Describe the solution you'd like Switch to an actively maintained clock. E.g. github.com/jonboulle/clockwork

Describe alternatives you've considered I've often used https://github.com/benbjohnson/clock in the past, but that appears to be archived as well.

albertteoh avatar Jul 27 '23 07:07 albertteoh

Being archived is not a strong reason to migrate off of by itself

Quinn-With-Two-Ns avatar Aug 01 '23 18:08 Quinn-With-Two-Ns

Being archived is not a strong reason to migrate off of by itself

I appreciate the response @Quinn-With-Two-Ns and yes, I agree, it's not a strong reason to migrate off by itself.

For context, the primary reason is usability.

Mocking time.Now() with https://github.com/facebookarchive/clock requires a bit more effort. An example in this codebase is:

env.mockClock.Add(startTime.Sub(env.mockClock.Now()))

With github.com/jonboulle/clockwork, it feels a bit more intuitive and readable:

env.mockClock = clockwork.NewFakeClockAt(startTime)

On top of that, I see that "github.com/facebookgo/clock" is only imported in a single test file, so migrating over should be fairly trivial.

It may even be a reason for mocking out other unit tests that are sleeping on a real clock.

Finally, the reason for reaching out was because we are using the temporalio/sdk-go which has an indirect dependency on github.com/facebookarchive/clock but, due to the usability issue mentioned above and it being archived, we decided to go for another clock implementation for our unit tests. So it's a bit of a shame that we can't just have the one clock dependency.

Again, not a pressing matter, just thought to discuss this to see if there's interest in the Temporal community, and happy to contribute a change too. I can also close this issue if there's no interest/appetite. 😄

albertteoh avatar Aug 02 '23 13:08 albertteoh

So it's a bit of a shame that we can't just have the one clock dependency.

while I understand, moving to different clock dependency could cause the same issue for other users currently using https://github.com/facebookarchive/clock

Given the simplicity of these mocks, I'd rather just get rid of the dependency and write some code in the SDK to replace it .

Quinn-With-Two-Ns avatar Aug 04 '23 14:08 Quinn-With-Two-Ns