README doesn't fully document "expiry"

[alert-debug]

In the Fields section of the README.md file, the Description text for the expiry row lists only UNIX timestamp, and probably needs to say Human-readable date, or milliseconds since the UNIX Epoch instead.

Actually I think it would be better to support seconds rather than milliseconds (since no one will be specifying a value more precise than 1 second, and I think most online tools use seconds since that is the definition used for UNIX time), so it might be better to say seconds since instead.

As this would be a backwards compatibility break, it's probably better to only document seconds but still detect and support milliseconds.

[jeemok]

I think that's a much clearer description 👍🏻 I've updated the README and published it in v3.1.2.

Regarding the seconds support, what if we support both seconds and milliseconds?

e.g. if a value of 1626604806766 (13 digits) is given, we will treat it as milliseconds; and if 1626604831 (10 digits) is given, we will treat it as seconds. I think it should be safe to assume by digits as for our use case here the expiry date is not meant to be used for a very distant time.

what do you think? @alertme-edwin

[alert-debug]

The README looks right, and I like the idea of supporting both seconds and milliseconds. My only concern about doing so is that it makes it harder to document (and implement and test). As no one needs to specify a value with more precision than 1 second, it might be confusing to offer that option, but I suppose there are some tools that will default to using the millisecond format of timestamp (and that keeps backwards compatibility).

[jeemok]

Sorry for the late reply to this! I had a look at this again and I think you're right, the README documentation is still kinda confusing. "Epoch" or Unix timestamp should represent seconds, but the current logic takes in the value and parsed it as milliseconds, using these functions:

• parsing library and function: dayjs(): https://day.js.org/docs/en/parse/unix-timestamp-milliseconds
• default function: new Date().valueOf(): returns the number of milliseconds between 1 January 1970 00:00:00 UTC and now

One nice thing about using milliseconds here is that we could parse both string or number (milliseconds value) to the function dayjs and it would handle it properly out of the box. If we are to change to seconds, then would need extra code handling and use dayjs.unix(seconds) function to handle it.

We should pick one or another to keep it simple and straightforward. I agree that no one would need the precision to the millisecond in our use case, but changing the value would be a breaking change and would need to release a major version so existing users are not affected.

I will update the README again to clarify we are using milliseconds now (remove the Epoch word to avoid confusion), and change it over to use seconds in the next major release.

[jdevalk2]

Just a note that in the documentation for expiry, the examples state

• '2020-01-31'
• '2020/01/31' ... As valid examples, this is misleading at best and plain wrong at worst

In JSON they are not valid entries, you have to use double quotes

  "1234": {
    "active": true,
    "notes": "blabla",
    "expiry": "2020/01/31" <= single quotes break here!
  }

Also better audit does not tell you if the JSON is invalid, it just continues like it does not have the file.

I would recommend changing the examples to have " double quotes.