Prefactor types into a separarate types package

[rabbbit]

This came up in #37 where we ended up in a dependency loop.

• main package was importing a specific rate limiter type
• both rate limiters needed to import Clock types from main package

By moving it to a separate package we avoid that loop.

If we decide to go ahead with #37, this will also be necessary
to have a "common config type" that can be re-used between all N
alternative limiters.

[prashantv]

This is going to make the API worse for users (I've seen this with zap / zapcore split as well).

Is there a big advantage in moving the implementations into separate packages vs having them unexported in the same package?

[rabbbit]

This is going to make the API worse for users (I've seen this with zap / zapcore split as well).

Is there a big advantage in moving the implementations into separate packages vs having them unexported in the same package?

In what way would it make it worse? I thought type aliasing would be invisible (?) - note we're still exporting only a single package unlike zap.

As to why:

• we currently have two implementations
• one has tests, one doesn't
• the tested one has a bug (lol)
• they have inconsistent options/constructors
  -- and there are some feature requests to change the interface ("Per", allow cancellation, etc)
• the mutex one is accepting Option that belong to the other type
• and, we're planning to create/test at least 3 more implementations (x/rate, ratelimitfx, slightly different mutex one)

So I having a single interface/constructor options exported, and but splitting implementations would:

1. be significantly cleaner,
2. make it easier to re-use tests.

It wouldn't be impossible with a single package, just more messy.

I was also toying with a thought of returning different implementations based on user params - that would require more testing though. For now, the mutex implementation seems to be faster for a single goroutine though, but not sure if that will hold.

We could:

• land this, see if we actually need various implementations before 1.0, potentially revert to a single package then.
• do some tricks with implementations accepting interface{}, and type asserting to different clock type (we already have ratelimit.Clock and ratelimit.internal.clock.Clock interfaces). Would only need to happen in a single place, so it wouldn't be horrible.
• try to run with a single package

I'm for (1), but I guess it depends on how much worse it would

make the API worse for users

What are you concerned about? :>

[prashantv]

In what way would it make it worse? I thought type aliasing would be invisible (?) - note we're still exporting only a single package unlike zap.

It's actually a little worse with type aliases to an internal package, since users will be forced to look at the API documentation for an internal package to understand the types fully. See how a type alias shows up in documentation:

https://pkg.go.dev/go.uber.org/zap#Field

There's basically no information, no method set, etc.

It wouldn't be impossible with a single package, just more messy.

I understand the need for 2 implementations, the differences between them, but I don't think any of that explains why a separate package for each implementation helps. The implementations are small enough to be separate files, what advantages do we get from moving them into separate packages?

land this, see if we actually need various implementations before 1.0, potentially revert to a single package then.

My preference is the other way around -- keep everything in a single package, and figure out workarounds for the dependency loop (which itself is actually caused by having multiple packages).

[rabbbit]

dependency loop which itself is actually caused by having multiple packages

Yeah, I understand what I'm causing;)

keep everything in a single package, and figure out workarounds for the dependency loop (which itself is actually caused by having multiple packages).

This I don't understand - if we keep everything in a single package, there's no loop, so there's no need for workarounds? ;)

need for 2 implementations

Might get to 4-5, at least temporarily.

what advantages do we get from moving them into separate packages?

The usual package benefits:

• separate namespace
  -- no NewMutexBased, NewAtomicBased, NewAtomicUnsafeBased, NewFake, just New
  -- no accidentally sharing structs, like Option being shared in wrong ratelimiters, as we have do right now
• separate go test and go coverage
• easier to enforce "every limiter should pass exactly same tests"
• I (maybe a bad thought pattern) find it easier to think about: we have a common interface/tests, and we have N implementations that should share nothing. Packages mean "share nothing".

All that could still be achieved in a single package, but splitting it up "is free" - I don't have to think about it, go will check for me and make sure I make no silly mistakes.

[rabbbit]

Spoke with @prashantv directly.

Agreed that type aliasing to a private package is not the way.

Package splitting it's still something I'd like to think about, but that can happen in #37.