docker-compose
docker-compose copied to clipboard
PDMLab
rm and stopMany methods have an inconsistent API
The signatures of the rm and stopMany methods is inconsistent with other methods that accept multiple services.
For all the other methods that accept multiple services the signature is services first, options second:
(services: string[], options?: IDockerComposeOptions | undefined) => Promise<IDockerComposeResult>;
Yet, for rm and stopMany the signature is different:
(options?: IDockerComposeOptions | undefined, ...services: string[]) => Promise<IDockerComposeResult>
I know that changing this signature is likely to result in a breaking change, but would you be willing to accept a PR to fix this?
We're still below 1.0.0 release, so everything is still subject to changes.
@Steveb-p any worries about this?
This sounds good actually. The function signature looks a lot better the way @johnytiago suggests.
Of course I'm worried about a breaking change that this would introduce. Ideally we would have a way of dealing with it, maybe by detecting that we're dealing with an old invocation (maybe by checking if the first argument is an array?) and swapping the arguments as necessary? :thinking:
A silent upgrade for function calls would be nice. Not sure if we allow services to be a string as well.
We could likely come up with a way to make this backwards compatible, but we'd still be introducing a special signature to this two functions only. The inconsistency we'd be introducing is better than the one we had before, though.
Regarding versioning and breaking changes, you could postpone removing the old signature for only when you feel like you have enough to go onto v1.0.0.
The easiest way of handling it would indeed be adding new functions, instead of trying to shove into the old ones. It would also help with types (or rather, we would not need to do any shenanigans with function declaration, as we're strictly declaring acceptable types currently, and we would probably need to mark lines that perform invalid type checks as ignored).
Just call them rm2 and stopMany2? Might be misleading regarding compose v2 maybe? However we don't have no v2 stuff there.
I don't think adding new functions is going to fix the problem either.
1 - the existing inconsistency between the many other *Many functions will still exist
2 - you'll be introducing yet another one. A different naming convention that you'd expect to act the same.
Plus, I don't think this is a problem enough that people need this feature out to resolve their problems. It's just unpleasant quirk one can easily fix on their end.
I'd say the best approach, that respects semantic versioning, is to maintain both signatures temporarily until the later gets deprecated on a major bump.
Something like this:
function stopMany( options?: IDockerComposeOptions, ...services: string[]): Promise<IDockerComposeResult>;
function stopMany( services: string[], options?: IDockerComposeOptions | undefined): Promise<IDockerComposeResult>;
function stopMany( services: unknown, options: unknown, ...rest) {
if (Array.isArray(services)) {
return execCompose('stop', services, options)
}
const _options = services
const _services = [ options, ...rest ]
return execCompose('stop', _services, _options)
}