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

fix: Export types directly from sqlstring dependency #3047

Merged
merged 8 commits into from
Sep 14, 2024

Conversation

ScrapsPrograms
Copy link
Contributor

Instead of defining new types, this change exports the sqlstring dependency's typing information directly.

Originally the idea was to just update the type of escape, but looking a bit further, this change seems to make more sense.
For reference, see this issue.

Signed-off-by: Scraps <123112608+ScrapsPrograms@users.noreply.github.com>
@wellwelwel
Copy link
Collaborator

wellwelwel commented Sep 11, 2024

Thanks, @ScrapsPrograms 🚀

To use @types/sqlstring, I think it's better to include it in the documentation, similar to what is done with @types/node:

```bash
npm install --save mysql2
npm install --save-dev @types/node
```


For example, you're re-exporting all types from @types/sqlstring, but as a development dependency, the final user will not have these types installed.

Also, it's important to note that this approach can imply a breaking change for TypeScript users, as they will need to install the @types/sqlstring manually to use these types 🙋🏻‍♂️

What do you think, @ScrapsPrograms?

This comment was marked as off-topic.

@ScrapsPrograms
Copy link
Contributor Author

ScrapsPrograms commented Sep 11, 2024

TLDR; The added @types/sqlstring may be moved to dependencies and the original overloads for the format function could be returned in a simplified form format(sql: string, args?: any | any[], stringifyObjects?: boolean, timeZone?: string): string, so the args?: object | any[] argument type does not cause conflicts.

I think it's better to include it in the documentation, similar to what is done with @types/node, instead of bringing @types/sqlstring to dependencies:

If I understand this correctly, the documentation here asks users to install the type definitions themselves. I would argue that, for a function exported by the node-mysql2 package, it'd make more sense for the typing to be exported alongside it, rather than requiring a separate user installation.

as a development dependency, the final user will not have these types installed.

This is true, and I have to admit I overlooked that. I can move them to the dependencies to have the typing included with the node-mysql2 typing definitions.

It's important to note that this approach can imply a breaking change for TypeScript users

This is understandable. Looking at the originally exported types and the types exported by sqlstring, they are mostly the same. Only the function format, which had a few overloads in the node-mysql2 package, provides a different type.
Most arguments in the sqlstring package format function are optional. Additionally, the type of the argument value: any in the node-mysql2 package's format definition is is different in the sqlstring package's definition: args: object | any[]. It may be a breaking change for TypeScript users if the provided value is neither an object nor an array of any kind. The other exposed functions cannot break any existing usage of node-mysql2's exposed sqlstring functions, as the typing is either the same or more limited.

To ensure no breaking changes are applied, an overload for the format function may be implemented to replace args: object | any[] with the originally applied args: any | any[]. I would like to have your thoughts on this matter.

@wellwelwel
Copy link
Collaborator

wellwelwel commented Sep 11, 2024

The added @types/sqlstring may be moved to dependencies

I'm not sure about this idea. Type dependencies shouldn't affect JavaScript users, for example by requiring @types/node, it will increase more than 2MB to MySQL2 install size.

  • Just to clarify, I like the idea of bringing in the development dependency and exposing its types, documenting it as a dependency to be installed manually for TypeScript users. Although I think this is ideal, the problem would be to cause a breaking change because of this.

To ensure no breaking changes are applied, an overload for the format function may be implemented to replace args: object | any[] with the originally applied args: any | any[]. I would like to have your thoughts on this matter.

For now, I think it's better to define the types directly in MySQL2. Let me know your thoughts 🙋🏻‍♂️

@ScrapsPrograms
Copy link
Contributor Author

ScrapsPrograms commented Sep 11, 2024

This seems to push the @types/sqlstring dependency towards becoming a peerDependency. AFAIK, this means it won't be included in the JavaScript version, as npm doesn't install peerDependencies by itself. With this change, the documentation will indeed need to be updated, as per your previous suggestion.
Sidenote: A different package manager, such as pnpm, might install peerDependency packages, but often this isn't a problem.
Let's call this possible solution 1.

Another option is to keep going the way node-mysql2 originally handled this: copy the types and handle it in the types package. For this PR, that'd just be a matter of copying the types exposed in @types/sqlstring and replacing the current types with that.
Personally, I'd prefer to find ways to have the dependencies handle their own typing, as changes can be updated through project dependencies rather than having to be updated manually when a change is requested, it seems a lot easier.

It seems the sqlstring package itself hasn't been updated since March 2022, with no PRs having been opened since a year prior to its last release. I would assume the typing information won't change anytime soon.

It may be a breaking change for TypeScript users if the provided value is neither an object nor an array of any kind.

This can be covered by replacing the type value: object | any[] with value: any | any[]. I've never used this format function myself, so I cannot say if anyone's passed a non-object, non-array type to it as the second argument or why they would be doing so (or if it works even).
Let's call this possible solution 2.

Lastly, and I'm not much a fan of this reasoning, but it must be mentioned to cover the last option I can come up with at this time, the typing information of @types/sqlstring exists of only one or two files, resulting in about 16kb added to the dependencies. It could be considered small enough to be irrelevant, and thus acceptable to be added. But again, I'm personally not a fan of that.

Unless a means of properly distributing the @types/sqlstring dependency can be applied without affecting vanilla users, my vote goes to option 2, which is to copy the typing information.

@wellwelwel
Copy link
Collaborator

wellwelwel commented Sep 11, 2024

It seems the sqlstring package itself hasn't been updated since March 2022, with no PRs having been opened since a year prior to its last release. I would assume the typing information won't change anytime soon.

sqlstring repository is closed only for members (no possibility to open issues or PRs).


Unless a means of properly distributing the @types/sqlstring dependency can be applied without affecting vanilla users, my vote goes to option 2, which is to copy the typing information.

For version 3.x.x, I think that define the types directly in MySQL2 is a better approach. So when there is a milestone for version 4, include the dependency point as a goal.

Just a note:

  • If a copy is indeed made, it is important to credit the source in a "header comment" 🙋🏻‍♂️

Signed-off-by: Scraps <123112608+ScrapsPrograms@users.noreply.github.com>
Signed-off-by: Scraps <123112608+ScrapsPrograms@users.noreply.github.com>
Signed-off-by: Scraps <123112608+ScrapsPrograms@users.noreply.github.com>
typings/mysql/index.d.ts Outdated Show resolved Hide resolved
typings/mysql/index.d.ts Outdated Show resolved Hide resolved
Signed-off-by: Scraps <123112608+ScrapsPrograms@users.noreply.github.com>
Signed-off-by: Scraps <123112608+ScrapsPrograms@users.noreply.github.com>
Signed-off-by: Scraps <123112608+ScrapsPrograms@users.noreply.github.com>
@wellwelwel wellwelwel merged commit 81be01b into sidorares:master Sep 14, 2024
72 checks passed
@wellwelwel
Copy link
Collaborator

Thanks, @ScrapsPrograms 🚀

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

Successfully merging this pull request may close these issues.

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