Rework method signature to allow just argumentName as an option #47
Conversation
terodox
changed the title
|
@jtheriault Do you have any additional feedback? Or can this be merged? |
Co-authored-by: Joe Theriault <[email protected]>
| export function coerce(value, Type, messageOrOptions) { | ||
| let errorMessage = messageOrOptions; | ||
| if (typeof messageOrOptions !== 'string') { | ||
| errorMessage = `${messageOrOptions.parameterName} should be coerceable to ${Type.name}. Provided value: ${JSON.stringify(value, null, 2)}`; |
There was a problem hiding this comment.
I think I'd rather flip this so that the default value is the parameter name and it can be overridden with an options object.
Despite technically maintaining the same signature, it would constitute a breaking change -- but I think for the better.
There was a problem hiding this comment.
I'm not sure I completely follow. Can you show me example of the two method signatures your thinking of?
There was a problem hiding this comment.
Picking this back up...
On deeper reflection, I think this is almost perfect as a pre-major change. Specifically I think this should be followed immediately by a new major version where the third parameter:
- is an optional object
- by default, implies an
errorMessageofCould not coerce a valid ${Type.name} from provided value: ${JSON.stringify(value, null, 2)} - can have
parameterNameproperty which changes message toCould not coerce ${parameterName} into a valid ${Type.name} from provided value: ${JSON.stringify(value, null, 2)} - can have
errorMessageproperty overrides the message entirely parameterNameanderrorMessageare mutually exclusive
If we think typing parameterName and/or errorMessage is too much, we could introduce short aliases of e for errorMessage and p for parameterName.
The only piece missing in the current PR would be to allow errorMessage to be specified as a property on the options object and not just as a backwards-compatability fallback string.
What do you think?
There was a problem hiding this comment.
I like the approach. I would argue that we should avoid the churn and just move to a new major version though.
I'm not sure we'd be helping a whole lot of people with the minor then major.
What do you think of that approach?
There was a problem hiding this comment.
I tend to prefer "ripping off the band-aid" to deprecating an API, but it's a common approach so it seemed responsible to suggest it 😉.
Considering parameter validation on the options object will direct the consumer's migration effort, I'm comfortable with just taking the leap.
That said, I think it would behoove us to survey some of our colleagues for their views to make sure we're not out-of-step.
There was a problem hiding this comment.
I love the optional object as the third parameter.
Addresses #46
I took a stab at an implementation that would address the desire for a default error message without breaking the existing function contracts.