Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

• typeorm-sql-js-example

  npm i https://pkg.pr.new/typeorm/typeorm@11766
  

commit: f0b8f3c

Nice!

Can you check if there's something related to authentication that can be removed? I remember that was the main change in the mysql2 client. MySQL itself deprecated and later dropped support for old auth methods, but MariaDB and cloud versions of MySQL (AWS / Alibaba) might still use the old authentication.

Another thing we might need to do is to flip the default for legacy spatial support to false (i.e. remove it from connection options in MysqlDriver.connect).

Can you check if there's something related to authentication that can be removed? I remember that was the main change in the mysql2 client. MySQL itself deprecated and later dropped support for old auth methods, but MariaDB and cloud versions of MySQL (AWS / Alibaba) might still use the old authentication.

I did not used mysql for many years not, so do not have the details. Can you point me to what exactly do you mean or maybe to some documentation? I also added it to the list in #11692, so it does not get forgotten.

Another thing we might need to do is to flip the default for legacy spatial support to false (i.e. remove it from connection options in MysqlDriver.connect).

This is already added to #11692

I would suggest we merge it as it is, and I will tackle remaning two points in separate PRs. I would also need some help since my knowledge of mysql is quite limited. I am typically using pg

Looks fine but needs testing.

Btw my ultimate-test-suite broke when trying to pull this PR, I will check that later.

Btw my ultimate-test-suite broke when trying to pull this PR, I will check that later.

Can you share your test suite? And why it is not part of the repo?

Can you share your test suite?

https://github.com/OSA413/typeorm-ultimate-test-suite

And why it is not part of the repo?

Because 1) it's my initiative, 2) it's in WIP state, 3) the dataset is very huge and 4) no one else took the initiative to help (but that's ok). I'm currently not considering including it into the main repo because it can be unstable (and you need some time to understand what's going on there). I personally use the test suite as a canary to test PRs I'm reviewing.

Because 1) it's my initiative, 2) it's in WIP state, 3) the dataset is very huge and 4) no one else took the initiative to help (but that's ok). I'm currently not considering including it into the main repo because it can be unstable (and you need some time to understand what's going on there). I personally use the test suite as a canary to test PRs I'm reviewing.

Thanks for sharing. The dataset is quite large, but rest does not seem extensively big. Yet it's indeed a but hard to grasp on the first look. And if the overal stability of the suite is in question, than indeed it's hard to rely on it.

Let me know what does your investigation brings as from my point of view this PR is ready to be merged.

Would this be a blocker to dropping support of mysql?

Would this be a blocker to dropping support of mysql?

Not necessarily. There is a #11274 PR which should fix it, so we should maybe work on this one too?

There is a #11274 PR which should fix it

I don't think this will fix the issue with truncated error log

@alumni @OSA413 what should we do about this then?

I don't think this will fix the issue with truncated error log

Sorry, I meant to point to #11273 not #11274... Looks like the original author have not time to work on it, so I can take it over if you think its worth it? And given @alumni initial approval it probably is.

it's worth it, you probably should continue working in the PR's branch and carry it to the point of it being good enough to merge into master (I haven't checked what's the solution to the problem with the truncated log)

I don't think #11273 is related in any way to this PR (besides affecting the same driver). There were some changes in the all drivers (including MySQL) related to event handling - that's why it has some merge conflicts - I wanted to fix and merge that PR since author is busy, but realized might take more than 5-10 min to fix the conflict. Feel free to check if you have the time (you can ignore my comment there about a promisify-like helper, it was just a suggestion).

I don't work with MySQL/MariaDB on a daily basis either, but if I change things I try to understand how they work before making the change.

For this PR, I would remove the legacy spatial support as default and put this information (and the client change) in some "migration guide" doc page (in the Guides section). Regarding auth, I would check how it works, but likely there's not much to remove, it was just a heads up that there is something that we should check (these things will not be done by code reviewers).

Deploying typeorm with    Cloudflare Pages

View logs

PR Code Suggestions ✨

Latest suggestions up to f0b8f3c

 protected loadDependencies(): void {
     try {
         this.mysql = this.options.driver || PlatformTools.load("mysql2")
+        if (Object.keys(this.mysql).length === 0) {
+            throw new DriverPackageNotInstalledError("Mysql", "mysql2")
+        }
     } catch (e) {
         throw new DriverPackageNotInstalledError("Mysql", "mysql2")
     }
 }

Previous suggestions

-                    `
-        }MySQL connection raised an error. ${error}`,
+                    `MySQL connection raised an error. ${error}`,

 connection.on("error", (error: any) =>
     logger.log(
         "warn",
-        `
-    }MySQL connection raised an error. ${error}`,
+        `MySQL connection raised an error. ${error}`,
     ),
 )

-                    `
-        }MySQL connection raised an error. ${error}`,
+                    `MySQL connection raised an error. ${error}`,

 connection.on("error", (error: any) =>
     logger.log(
         "warn",
-        `
-    }MySQL connection raised an error. ${error}`,
+        `MySQL connection raised an error. ${error}`,
     ),
 )

-                    `
-        }MySQL connection raised an error. ${error}`,
+                    `MySQL connection raised an error. ${error}`,

 connection.on("error", (error: any) =>
     logger.log(
         "warn",
-        `
-    }MySQL connection raised an error. ${error}`,
+        `MySQL connection raised an error. ${error}`,
     ),
 )

coverage: 80.797% (+0.03%) from 80.769%
when pulling f0b8f3c on mysql2
into ef9e4b6 on next.

It looks good, did you use find replace approach?

No, search and manual replace/changes. Why do you ask?