Typescript conversion

[evert]

Hi @sidorares !

I was curious if there's an interest in a total typescript conversion of this project. I started this as an exercise/experiment and noticed the code is hella clean which makes this relatively easy (just a big grind, no big refactors).

If this is a goal of this project to eventually make this switch, I can do the conversion with a future pull request in mind, but if the intent is to keep this a Javascript project I will just keep my fork and make more dramatic changes.

[wellwelwel]

I believe it would be interesting to list both positive and negative points 🙋🏻‍♂️

A question: Would the tests/tools also be converted or just the main code?

Before the v3.2.1 (where it was not possible to compile projects made with TypeScript using MySQL2 without skipLibCheck), I started a fork with this goal in mind, but ended up turning it into an independent project (ORM).

---

Some notes for this discussion:

• Currently, there are ~48 PRs that would need to be converted to TypeScript.
• At this moment, the issue Incorrect typings lead to pool.destroy is not a function #2667 is the only one directly related to typings.
• Direct dependencies that have no typings:
  • generate-function
  • named-placeholders
  • seq-queue
• Dev dependencies that have no typings:
  • assert-diff
• Maybe a major bump is needed for TypeScript users

[evert]

I may not be the best person to list all the pros and cons. I think the benefit for me is that I'm intending to write a new user-facing API for MySQL using Symbol.iterator, and I want to try my hand in analyzing SQL strings and generating types based on the database schema and sql queries to try to bring raw SQL queries into the type system.

Typescript gives me a very degree of confidence during refactoring that plain Javascript does not give me. So starting off here both lets me give a deeper understanding/appreciation for the existing codebase and will make making changes less painful. I figured since I'm doing this I might as well see if this project would benefit my grind ;)

I suppose another benefit is that it's much harder for the typings and code to get out of sync, leading to issues like #2667.

A question: Would the tests/tools also be converted or just the main code?

I did not intend to touch the tests, because this gives me a great check ensuring that my conversion didn't break anything. I personally like having my tests in Javascript because so many tests check edge-cases and Typescript tends to complain a lot.

A final disclaimer: I don't finish every project I start

[sidorares]

Good point about backlog of PRs / in progress branches that someone will also have to convert after big refactor

In general I don't mind this but definitely not high on my priority list.

If we speak about "big refactors" I'd rather separate very core protocol ( packets serialisation / deserealisation, raw stream -> raw packets deframing layer, maybe some commands abstraction, maybe using high level packet binary structure description language where possible ) and "user facing api" modules

Also, if typescript refactor happen vanilla JS users experience is still a top priority - I want this library to "just work" for node/bun/deno users, regardless of ESM or CSJ and regardless if there is any typescript setup or not

[sidorares]

I want to try my hand in analyzing SQL strings and generating types based on the database schema and sql queries to try to bring raw SQL queries into the type system.

I'm curious to see this happen but this is definitely out of scope for this module. The focus here is "make communication with server performant, reliable and have decent DX". While there is a bit of "understand what communication happens between client and server and make smart decisions based on that" ( for example, we react on charset change sql because of the changed variables returned as response flags ) we don't try to parse sql in any way.

[evert]

Good point about backlog of PRs / in progress branches that someone will also have to convert after big refactor

In general I don't mind this but definitely not high on my priority list.
If we speak about "big refactors" I'd rather separate very core protocol ( packets serialisation / deserealisation, raw stream -> raw packets deframing layer, maybe some commands abstraction, maybe using high level packet binary structure description language where possible ) and "user facing api" modules

Honestly that would be amazing. If this existed today I would just have reached for the core protocol package =)

Also, if typescript refactor happen vanilla JS users experience is still a top priority - I want this library to "just work" for node/bun/deno users, regardless of ESM or CSJ and regardless if there is any typescript setup or not

My assumption is that after this work that everything will still just work as-is. Hoping that there's no BC break break.

I'm curious to see this happen but this is definitely out of scope for this module.

Yeah my intent was to use this library to build/experiment with a new user-facing API, but didn't assume that work would be merged in. Just want to give back this specifically the Typescript conversion (provided there was interest). Line count I'm about 50% done.

If I finish the work, I'll submit the PR and you can decide what to do with it. And if you decide it's not worth it, that's fine too.

[evert]

I've found 1 thing so far I have a hard time understanding. This function:

node-mysql2/lib/parsers/string.js

Line 10 in a2dd627

exports.decode = function (buffer, encoding, start, end, options) {

takes an encoding argument. This argument appears to be required to be a string as iconv-lite requires this. However, there are many code paths in this library for which this argument is not set, such as:

node-mysql2/lib/packets/text_row.js

Line 14 in a2dd627

columns.push(packet.readLengthCodedString());

(which calls readLengthCodedString without an encoding argument, which in turn calls that original decode function). I've been stuck on this for too long.

I understand this is not worth your time, esp since you're not sure about this, so feel free to politely decline if this is (or future) questions are a nuisance.

[sidorares]

@evert feels like its a relic and not used anymore. This is probably a code that was used from "non-jit parser"
Note that we might still need it ( with encoiding issues fixed/backported ) for #2289

[evert]

Interesting, so are you saying that the whole TextRow class is probably not used anymore. The calls to readLengthCodedString() seem pretty critical to that class. TextRow itself is referenced from connection.writeTextRow

[sidorares]

I believe connection.writeTextRow is used in server examples end test code. We should probably fix the encoding parameter usage in text_row.js