Allow passing in custom value handlers

[anton-johansson]

A suggestion for an implementation of #1170.

Related @types-change here:
anton-johansson/DefinitelyTyped@9c95657

I tried to do some generics-magic to automatically capture the correct type to parse, but I did not get it working properly... It should be possible, but I'm still working my way through TypeScript generics (I'm coming from Java).

Usage example:

import {registerValueHandler, DateTime, SmallDateTime} from "mssql";
import {toLocalDateTime} from "../utils/date";
import {LocalDateTime} from "@js-joda/core";

registerValueHandler<Date, LocalDateTime>(DateTime, value => toLocalDateTime(value));
registerValueHandler<Date, LocalDateTime>(SmallDateTime, value => toLocalDateTime(value));

As you can see, I have to manually specify Date, which isn't desirable. I'd love to fix this with proper TypeScript generics capture, but as I mentioned above, I couldn't get it working.

Also, there would be something similar for parameters, but I thought I'd start here.

[avner-hof]

@anton-johansson , @dhensby - what's the status of this PR?

Since mssql version 6, Dates are returning from mssql as JS Date object and not strings. This is a breaking change and we (and from what I've seen others as well) would like to have the option to keep the returned JS date type as string.
By looking at the PR changes I'm trying to understand if this PR will add the option to customize the returned JS data type for each mssql data type such that running request.query will return the desired type?

[anton-johansson]

@avner-hof: Well, I'm still waiting for a response from a collaborator/maintainer. :)

[avner-hof]

@dhensby ??

[dhensby]

Hey, sorry for not giving much feedback on this. I'm not sure this is an elegant solution, but then again, I'm not sure there are any elegant solutions.

Can we add some tests (just an example of a type being re-cast as expected), then we can get this in.

@anton-johansson - have you been using this in your projects?

[avner-hof]

@anton-johansson - anything new with this?

[anton-johansson]

No, sorry, I haven't been using this in my projects yet, but I'd like to.

I agree it's not the most elegant solution. It would be great if you could hook in "earlier" in the flow, and control how the values are parsed directly from the SQL result, but I'm not sure what kind of options we have there.

I bumped into another issue now, where BIGINT is read as string (I assume because they cannot fit into a number). In those cases, it would be great to be able to say that I want BIGINT as BigNumber (from an NPM package) or something similar.

@dhensby If you think the solution is acceptable (even though not so elegant), then I can continue with some tests, etc.

[dhensby]

I bumped into another issue now, where BIGINT is read as string (I assume because they cannot fit into a number). In those cases, it would be great to be able to say that I want BIGINT as BigNumber (from an NPM package) or something similar.

yes - I think this is one of the most compelling reasons to add this kind of feature, it gives the developers some control over these kinds of values which I think is too opinionated for the library to do itself (and not very flexible, either).

If we can get a test and an update to the README, that would be great. I also think we need a way to clear the value handlers (possibly just for the tests, but it's helpful to be able to clear them anyway)

[avner-hof]

Hey @anton-johansson - any progress with this great PR?
I think many of us using this package are stuck with older versions due to dates returning as JS Date object instead of string as it used to be, and can't upgrade to the latest version containing many other needed fixes. This PR will allow upgrading to the newest release

[dhensby]

@avner-hof it really shouldnt be too hard for your app to convert date objects to strings if that's what you need (a simple iteration over values and converting any that are instance of date to strings).

But if there isn't progress on this PR soon I'll look to take on writing the tests or maybe refactoring it a little. But we are talking in a month or so, I think.

Others can also take this on if it's an important feature for them.

[avner-hof]

@dhensby - That's more complicated than that, clearly parsing date object to string is simple, but we're talking about a system with hundreds of thousands of code lines for a company which serves many customers and companies.
Manually changing all the lines that deals with dates is very risky and error prone whereas converting dates to strings in an infrastructure level adds significant time complexity and isn't an option.
And unfortunately all the bug fixes aren't backported (I don't know if that's even possible due to tedious driver is upgraded)
That's why we really need this

[dhensby]

I think you're misunderstanding. Of course going through and changing all the application logic to expect a different data type is a bad idea.

Your database layer/logic where your queries are made can convert the types in a single place without adjusting your application logic at all.

[avner-hof]

@dhensby - I did understand, and that's why I wrote that converting dates to strings in an infrastructure level adds significant time complexity. Since you don't know what and when is returning from a query in the DB layer, it means going over every field that returns from every query, checking its type and converting it to a string. We did that. Yes it works, but it adds a significant time complexity due to the need to go over every field

[dhensby]

But that's exactly what happens at this layer too... Every field is evaluated for its type and acted on. There's nothing different. If it's adding significant time complexity to process your DB queries then there's a bigger problem afoot.

[avner-hof]

@dhensby - By looking at this PR I don't see there's another loop created to go over all the fields again and convert their values. Look at the valueCorrection function, it just adds some logic to calls the handler in case there is one for the handled field.
If I'm doing it by myself later on my DAL layer it add time complexity of O(n^2) since I need to go over again on every field and convert it myself. That's why this PR is important and the conversion should occur on the library level and not in the code of the library consumers

[dhensby]

Ok - I'm looking to get this into the v8 release (which I'd like to get out this month).

Things I think need doing:

• Add same feature to msnodesqlv8 / make it global
• Add test
• Add a way to remove a registered handler (good for testing and flexibility)
• Documentation
• Changelog

[anton-johansson]

@dhensby, glad to hear it! I'm having a bit of a busy couple of weeks in front of me so I'm not sure I'll have the time. I'll try and see if I can find some. :)

[dhensby]

@anton-johansson no worries - I was planning on picking it up myself :)

[anton-johansson]

@dhensby, oh great! 🙌

[dhensby]

I'm closing this in favour of #1356