Rely purely on moment to determine input validity

[brockpetrie]

Removes our falsey check on input on line 38. This allows for undefined inputs, which are considered valid by moment and treated the same as calling new Date().

This also removes the console warning in that same if block.

Fixes #75, closes #81 and closes #91. Breaking change! Any thoughts on this, @BrockReece?

Thanks to @bitencode for doing the initial legwork.

[BrockReece]

The code changes make sense, the only thing is that this contradicts the changes we made for #29

[brockpetrie]

That is a pretty big contradiction, isn't it... How do you feel about it? I think I've come full circle: respect how Moment handles inputs, and either proceed or abort depending on Moment's thumbs up.

[BrockReece]

I have no strong feelings either way to be honest, though a lot of feedback we had in the issues was around not firing the console warning rather than displaying nothing if the input is falsey.

[bitencode]

@brockpetrie - I've been testing this branch with our system and it works well. I agree with your assessment that undefined should be a valid input and run the filter as if the time were "now". In our app we don't use (or pass to moment) undefined times, but I think that's a good option. We do use null a lot as timestamps when an event has not occurred (which is where we were getting all the console warnings).

For instance, this snippet of markup:

    td.no-data-dash {{ item.instanceType }}
    td.no-data-dash {{ item.version }}
    td.no-data-dash {{ item.startedAt | moment() }}
    td.no-data-dash {{ item.stoppedAt | moment() }}

Helps to produce the following:

Everywhere there are dashes in the UI is where there was null data - in this case no "Last Launched" or "Last Stopped" dates. The page that's clipped from actually has dozens of the main rows with expandable details I'm showing here. Results are potentially multiple hundreds of detail rows, many which may have null timestamps for specific events. We use this same pattern though-out our application, so not having the console warnings is very nice 😉

It also means that it's important to have no output if the input is null (which this PR keeps) so that the .no-data-dash CSS class can do its thing if the td is empty.

There are situations where we end up rendering a lot of data that has had few or no events yet - ex:

That brings me to the only suggestion I have for this PR. In #81 at the top of the function I had a null guard (it also guarded undefined (which we don't actually want). But, I noticed when I was timing the page renders that for a page with many null time values (which we have a lot of) that short-circuit speeds up that page. It's up to you, but adding if (input === null) return; at the top apparently avoids a lot of unnecessary processing.

Either way, we'd very much like to have this be the default behavior, so you get my 👍 for merging 😄

[bitencode]

Though I was just re-reading your comments from 3 days ago about my PR #81 going too far by guarding the invalid inputs... so yeah - either way, letting moment do the processing means you'll get whatever it does at a small performance penalty for "invalid" inputs (null, empty string). Guarding those values at the top will give you a performance bump, but I'm not clear on how much and it's possible our status pages that contain a lot of as-yet pending events are unusual.

I'm also happy to close #81, but it looks like when you merge this it will automatically close, so leaving it for now.

Thanks!

[bitencode]

Have you thought about merging this at all? We are still using our modified fork. Someday I'd like to switch back to your npm package instead.

Thanks!

[kamerat]

Leaving this PR hanging like this is torture. So close, yet so far.. ☹️

[cManfredi]

Any updates about this PR?

[BrockReece]

I think we should just bite the bullet and go with this.

[BrockReece]

@brockpetrie Are you happy for me to merge this in and release?

[bitencode]

Yes! One of the Brocks please merge this 😁 (coming up on one year since I did the first pull request for this).