feat: deprecate consumerVersionTag and providerVersionTag. Fixes #218

[mefellows]

• Adds consumerVersionTags and providerVersionTags as options that only accept arrays. I think accepting a string or array is unnecessary.
• Logs deprecation warning if consumerVersionTag and providerVersionTag is used
• Supports both options until a major version bump.

[TimothyJones]

Unless I'm missing something, I can't see where the two options are used by the lower layer.

It would be good if we didn't muddy the type definition with the two types too - maybe we should have a separate DeprecatedVerifierOptions which is unioned in? That way we could send only the new complete type down to lower layers after validation.

I think accepting a string or array is unnecessary.

Shouldn't we be permissive in what we accept, and restrictive in what we send? ;)

[mefellows]

Unless I'm missing something, I can't see where the two options are used by the lower layer.

You are right, I didn't push that line up inexplicably. Fixed.

It would be good if we didn't muddy the type definition with the two types too - maybe we should have a separate DeprecatedVerifierOptions which is unioned in? That way we could send only the new complete type down to lower layers after validation.

Would you mind showing how that would help? Unioning it in will still show it up in the type interface AFAIK. I thought about using decorators but that may not be portable, and won't help non-TS/JS users anyway. So the warn was the best I could come up with. Agree, it's a bit shit - alternatively I bump a major version and just remove completely.

I think accepting a string or array is unnecessary.
Shouldn't we be permissive in what we accept, and restrictive in what we send? ;)

With that argument, we should accept strings for everything and do the coercion behind the scenes. But seriously, do you think we should do both? It's a pluralised property now, so for me, it makes sense for it just to be an array. But if you feel strongly...

[TimothyJones]

Would you mind showing how that would help?

You're right that the compiler would see the unioned type, but for users reading the type, I think it's clearer to be able to see the deprecated options separately.

I think it's largely readability / layer separation. I think there are two types:

• What we expose to callers (kind of the DTO)
• What we pass to the lower layer (kind of the domain entity)

What's convenient to keep each layer simple might be different. For example, it's not convenient if the layer that consumes the options knows that there once was another way of specifying the options. Similarly, it's not convenient if the lower layer knows that you also can provide a string instead of an array (I've been unrelatedly thinking about clearing this up - kind of marshalling a user-friendly structure into a clear easily mappable domain config structure).

With that argument, we should accept strings for everything and do the coercion behind the scenes.

Well, not exactly :) I think APIs should only accept not-quite-as-written if it is completely unambiguous. But also- yes. I do think that we should coerce where it is clear what the user meant.

It's a pluralised property now, so for me, it makes sense for it just to be an array.

If you're saying it's plural so we shouldn't accept a single string, I think the same argument could be made for not accepting consumerVersionTags: ["oneTag"], (which I don't think is a strong argument).

My view is that APIs should be hard to misuse, and that programmers shouldn't have to do work because the computer exists. In this case the "extra work" is putting in [], so I would say my summary is:

But seriously, do you think we should do both?

I do.

But if you feel strongly...

I don't :)

[TimothyJones]

All the discussion above is why I didn't get to this change yet, though :)

Btw, I can see that you pushed again, but for some reason I can't see the change. Maybe it's github caching the diff or something.

[mefellows]

OK, given the change was so simple I've gone with your suggestions Tim. I'm not 100% liking the VerifierOptions & DeprecatedVerifierOptions littered everywhere, but I think having a reminder will mean we actually will get around to deprecating it.

I could have gone with something like export type CompleteVerifierOptions = VerifierOptions & DeprecatedVerifierOptions but I wasn't sure that was going to have the intended interface result.

Feel free to change as you will if you're picking up the WIP pacts anyway.

[TimothyJones]

Ah - making it the way I was thinking of was messier than I was thinking of - I can see why you were asking for an example 😂

I was hoping we could export a type which was a union of deprecated and current (which I've now done), and inside the constructor convert it to just the current options type (which turned out to be kind of messy with the rest of the way the code is structured).

I'd like to rethink this whole layer at some point. It seems a lot heavier than it needs to be, but I've left that for a rainy day.