Thinking about it, can't we just use the existing unit without adding any new api ?

@Marsup PR has been updated and downstream merged with your recent commits to master.

I've removed the approach that uses unit and simplified the new timestamp constraint implementation.

So what happened to my last comment ?

I'm not sure I understand what you mean? Please explain.

I don't think a new API is needed, just using the existing unit.

I agree that replicating the unit API within the date type is not needed, however I respectfully disagree about the need for a new API for timestamps. The date type has an explicit iso constraint and should have an explicit timestamp constraint as well.

This PR is clean, functional and has 100% test coverage.

If you have objective reasoning for a different approach on handling timestamps let's discuss it. For instance, getting rid of the type parameter in the timestamp API and using the built in unit to handle the multiplier. This would result in something like the following for unix timestamps: Joi.date().timestamp().unit('seconds').

Joi.date().timestamp('unix') is essentially the same as Joi.date().unit('s') except you're adding a new API and a new concept of "unix timestamp" that not everyone might be familiar with. Joi.date() just defaults to unit('ms') when it's a number.

ISO is a strict format applied on a string hence its special treatment, a number is not timestamp by itself, any number is a timestamp. Actually, if you want a strict validation on that, I'm just now seeing that this could already be achieved by a Joi.date().format('X') or Joi.date().format('x').

using the built in unit to handle the multiplier

I do mean that the unit should translate to a multiplier.

FWIW I'm not questioning the quality of the PR here, just the content.

@Marsup I am pretty sure it is a bad idea to change the parsing of a date depending on the .unit() value, given that it is only for annotation purposes.

Also, Joi.date().format('X') won't work for fractional timestamps.

It's been for annotation so far, is there any valid reason not to use it as it already exists ?
Fractional as in x/y ? Moment seems to support floats according to their docs, I didn't test.

Fractional, as in 123.45, which Joi never passes on to moment.

Oh yeah, but with this patch it would.

While using unit to set the timestamp multiplier is a valid approach, what are the reasons we should not add an explicit timestamp constraint?

To me, Joi.date().timestamp() is more descriptive about the function of this constraint vs Joi.date().unit('s').

I don't mind the timestamp per se, the flag behind it seems like a useless duplication of unit though. As common ground I'd propose timestamp takes whatever it can and translate it to unit, OK for everyone ?

That can work. So to confirm, is the following acceptable? If so, I'll update the PR accordingly.

The timestamp constraint will apply a numeric regex (/^[+-]?\d+(\.\d+)?$/) and any argument passed will set the existing unit flag (which will be used to control the multiplier).

So for instance…
Joi.date().timestamp('seconds') or Joi.date().timestamp().unit('seconds') will explicitly test for numeric values AND set the existing unit to seconds which will as a result trigger the multiplier. In other words, a stricter validation (eg non-timestamp values will fail).

Joi.date().unit('seconds') will trigger the multiplier, but not force the numeric values regex. In other words, a less strict validation (eg non-timestamp values will not fail but the multiplier may not be used based on the value format).

I'd still argue against changing the meaning of unit(…). If you decide to go ahead, remember to flag this as a breaking change.

Honestly, I'm leaning that way as well. Avoiding unit feels cleaner and follows a better separation of concerns. "Piggy-backing" on unit introduces cross-over between the APIs and potential complications down the road if the functionality/purpose of one changes.

To me, the question should come down to the timestamp(...) parameter using a concept of type vs time segment – unix/javascript vs seconds/milliseconds. Type feels more descriptive, but time segment seems like it would allow for greater flexibility with the multiplier and value conversion.

Back-channeled a bit with @kanongil, let's stick to the current form, gonna review now.

Thanks for the review and line notes @Marsup!! I'll work on these changes and let you know when it's updated.

@Marsup, PR is updated. Good call on adding some additional checks for ambiguous values. I've also updated the date.base language to be timestamp agnostic.

When all looks good let me know and I'll rebase down to a single commit so it keeps the history short.

PR has been updated and rebased

Thanks for the hard work.

You're welcome!! Thanks for the feedback and reviewing my pull request!

👍

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.