Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exposed .escape() functions exposes but don't cover the MySQL original typing #3030

Closed
ScrapsPrograms opened this issue Sep 10, 2024 · 3 comments · Fixed by #3047
Closed

Comments

@ScrapsPrograms
Copy link
Contributor

During some experimentation, I found that the type definition of the .escape(value: any) function isn't correct. Currently, for all locations, it's described as .escape(value: any): string.

The function exposed by the package, however, has a different interface: .escape(value: any, stringifyObjects: boolean, timeZone: string | "local"). From what I've found so far, this applies only to the loose exposed function, not to any of the class defined methods, which seem to wrap this function by abstraction.

For reference, the classes with a .escape(value: any) method are in Pool and Connection.

I'm willing to put a PR for this, if you agree the typing information should be updated, or if an abstraction should be applied to the loose exposed function similar to the class defined .escape methods.

@wellwelwel
Copy link
Collaborator

Thanks for the report, @ScrapsPrograms 🙋🏻‍♂️

I'm willing to put a PR for this, if you agree the typing information should be updated, or if an abstraction should be applied to the loose exposed function similar to the class defined .escape methods.

Please, feel free to contribute 🤝

@wellwelwel
Copy link
Collaborator

.escape(value: any, stringifyObjects: boolean, timeZone: string | "local")

Just a tip:

Instead of string | 'local', you can use 'local' | string & {}. It will allow using a dynamic string, keeping the auto suggestion support, e.g.:

Screenshot 2024-09-10 at 05 43 04

@ScrapsPrograms
Copy link
Contributor Author

TLDR; I wish to move typing responsibility of directly exported sqlstring functions from this package to @types/sqlstring.

I see there's a few more functions in the sqlstring package that're exported as-is. My idea is now to instead let the types defined in @types/sqlstring be exported through the node-mysql2 definitions. This will move responsibility for having the typing proper be with the @types/sqlstring package, meaning for node-mysql2 it can be assumed to be correct, and typing improvements will be applied automatically when the dependency version is bumped.

This should be type safe, given the implementation is literally as follows, where const SqlString = require('sqlstring').

exports.escape = SqlString.escape;
exports.escapeId = SqlString.escapeId;
exports.format = SqlString.format;
exports.raw = SqlString.raw;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants