feathersjs-ecosystem / feathers-authentication-management

Adds sign up verification, forgotten password reset, and other capabilities to local feathers-authentication
https://feathers-a-m.netlify.app/
MIT License
247 stars 98 forks source link

new Date() wrapper #87

Closed sylvainlap closed 4 years ago

sylvainlap commented 6 years ago

This PR wrap every Date.now() into new Date(), to work with Date instead of String, and fix https://github.com/feathers-plus/feathers-authentication-management/issues/86.

eddyystop commented 6 years ago

Wouldn't this be a breaking change, making existing records incompatible?

There is also the issue of multiple tests now breaking.

sylvainlap commented 6 years ago

Yes, it could be a breaking change, but I really think that working with new Date() is a best practice that working with Date.now() for token expirations.

Besides, I'm pretty sure that tests were breaking before my PR, and these breaks are not PR-related.

vonagam commented 6 years ago

What about making it configured with option which defaults to current behaviour, so that it is not a breaking change?

For example, option dateFormat with variants number and date.

Add new helper like this:

const getDate = (milliseconds, dateFormat) => {
  if (dateFormat === 'number') {
    return milliseconds;
  } else {
    return new Date(milliseconds);
  }
};

And wrap all dates in it like getDate(Date.now() + options.delay, options.dateFormat).

eddyystop commented 6 years ago

I'm planning to rewrite this repo this year. async/await instead of promises, prop values compatible with all DBs, etc.

I'll consider all outstanding issues at that point.

claustres commented 4 years ago

As this PR is pretty old closing now, please reopen based on new code if required.