haha, the NaNs is definitely not expected; looks like an edge case no one really ever checked :) My only real thought on this was "null vs zero date time", but it seems like the recent MySQL server versions are trying to just kill the zero dates. Returning null seems fine to me. @sidorares ?

I'm using "Invalid date" for this:

> var INVALID_DATE = new Date(NaN);
undefined
> INVALID_DATE
Invalid Date
>

that way it's still a Date object, but does not point to any valid date value

Oh, @sidorares , this is for the opposite direction: JavaScript -> SQL (rather than SQL -> JavaScript).

oh, sorry, get it now. What's expected behaviour for server then? Receive it as 0:0:0 date?

So given

var date = new Date(NaN)
var sql = SqlString.format('SELECT * FROM archive WHERE date >= ?', [date])

here is what this module does right now:

SELECT * FROM archive WHERE date >= '0NaN-NaN-NaN NaN:NaN:NaN.NaN'

And this PR currently proposes that becomes

SELECT * FROM archive WHERE date >= 'null'

In fact, I actually thought this made the above date >= NULL, but I realized that looking at the included unit test, it is actually date >= 'null', which does seem weird on second thought.

What I was originally mulling was should the above be date >= NULL, date >= '0000-00-00 00:00:00', or something else? I think NULL would be OK, but not sure. I definitely think that the current behavior date >= '0NaN-NaN-NaN NaN:NaN:NaN.NaN' seems very strange and honestly a bug.

I think new Date(NaN) convert to NULL will be better.

DateTime field can be NULL.

CREATE TABLE demo (
    id INT IDENTITY(1,1) NOT NULL,
    fooDate DATETIME NULL,
    PRIMARY KEY (id))

NULL would make more sense than 'null' imo
The 0 date is debatable but if mysql is moving away from it I would think NULL is preferable.

Cool, so I thought that the PR doing 'null' vs NULL was just an accident and I can always fix that up on merge unless @AdriVanHoudt does it first :)

But, I guess it sounds like everyone is in agreement on the NULL vs 0000-00-00 00:00:00 thing, yes? Going the path of NULL?

I do return null so not sure how to make it to NULL vs 'null'

Do I need to return 'NULL'?

I would not return NULL but rather an exception or some kind of error that makes it clear that an invalid date was passed.

The intention of this lib is to escape js stuff to valid safe MySQL syntax.
It also ignores functions in objects and doesn't generate errors on its own anywhere.
The responsibility of what you pass into the lib is for the user imo.
I feel like this lib should do its best to make sure it escapes/converts everything and converting to 0NaN-NaN-NaN NaN:NaN:NaN.NaN doesn't seem very helpful.

Right, at least in the current form, the intent of this lib is to escape what was given in some way and try to avoid throwing.

I do return null so not sure how to make it to NULL vs 'null'

I can always fix it :) but when I looked, the reason is that in the main escape function, your null is passed to the string escape function, as it currently assumes all those functions will return a string. We would just need to add a check for null return value and return 'NULL' from escape (sorry for the brevity; typing this on a phone).

Also, on the NULL vs zero date, I was thinking about it and had some additional thoughts I'll type up a bit later when I get to a computer :)

@dougwilson I'll wait until you typed your comments then (and would returning 'NULL' manually not also fix it?)

@dougwilson friendly ping for your thoughts ;)

Hi @AdriVanHoudt , sorry, I didn't forget, but just kept running out of times in my sit downs :) So here are my thoughts I wanted to lay out:

I feel generally that null and new Date(NaN) convey different information sometimes, and that if you do want null, that already has a mapping here (null -> NULL), and one should just pass that in. That being said, I also feel like it's someone important to (at least with the defaults) have round-trip stability with the mysql type castings. The castings will not actually return an Invalid Date object, instead simply just returning the date/time as a string, so there is no good example to base thing change on in that regard.

Since NULL can mean undefined / unknown / similar in MySQL, doing that for invalid date I guess kind of works.

Anyway, those are my thoughts. Not really arguing for any change, but wanted to write them down here, especially if someone comes questioning the decision :)

Unless there is any argument, we'll just land this as NULL by 10/28.

No problem just wanted to make sure ;)
I see your point.
To me it depends really on the intent of the lib. If you want to parse as good as possible it should be NULL (I don't know if there are other cases like this in the lib)
If you want to convert to a string as good as possible it seems more logical to follow util.inspect(new Date('a')) which resolves to Invalid Date (which in itself is way clearer than 0NaN-NaN-NaN NaN:NaN:NaN.NaN when getting errors)

Sorry, lost track of this for a bit :) Fixed it up to return NULL instead of 'null'.