node-mysql2
node-mysql2 copied to clipboard
Adapt to Cloudflare Workers environment
try to close #2179
Summary
- To fix the error
window is not found
, replace nodeTimers
with web timers - To fix the error
Code generation from strings disallowed for this context
, using the static parser in PR #2099 - To fix the error
crypto.hash() is not a function
, polyfill nodecrypto
withwebcrypto
, and modify the functions which directly/indirectly called thecrypto
library as async function.
TODOs:
- [x] mysql_native_password auth plugin
- [x] caching_sha2_password auth plugin
- [x] sha256_password auth plugin
- [x] static_text_parser (will be finished in PR #2099)
- [ ] static_binary_parser (will be finished in PR #2099)
- [ ] add integration tests
- [ ] repalce
pg-cloudflare
withmysql2-cloudflare
?
Usage
For users, they need to:
- Add
node_compat = true
flag to polyfill the missing node modules - Add
useStaticParser
flag to force mysql2 to use static parser.
For example: https://github.com/Mini256/mysql2-cloudflare-test/blob/1f339a85f4457b696f122503051991326dbc23dc/src/index.ts#L30
This PR can run on Cloudflare Workers with the following database:
- MySQL 5
- MySQL 8
- TiDB Serverless Cluster
- PlanetScale
@sidorares could you please approve to run the GitHub action?
@shiyuhang0 @Mini256 thanks for working on this! Could you document simple steps I can follow to try it myself?
@shiyuhang0 @Mini256 thanks for working on this! Could you document simple steps I can follow to try it myself?
https://github.com/sidorares/node-mysql2/pull/2289/commits/cc3458f23b222e49856d5acc14faa6f99f0671ee
This commit adds a step-by-step tutorial. Since it's user-facing, it needs a new release of MySQL2 including this PR.
it's better to follow the develop guide
in the doc to test locally without a new release.
CI is green. Maybe we can ignore the failure of CodeQL action, because the error is reported in the code that originally existed (just the location has changed)
As for the static parser. We'd like to merge your PR https://github.com/sidorares/node-mysql2/pull/2099 rather than cherry-pick it into this PR. What's your opinion? @sidorares
The static binary parser is also needed to make things work. Code generation from strings disallowed for this context
still occurs when code goes through the binary parser.
As for the static parser. We'd like to merge your PR #2099 rather than cherry-pick it into this PR. What's your opinion? @sidorares The static binary parser is also needed to make things work.
Code generation from strings disallowed for this context
still occurs when code goes through the binary parser.
I can probably merge it first, together with added binary protocol non-jit parser.
I don't like the flag name "useStaticParser", it reflects more implementation details rather than intention, but don't have better ideas. Can you think of a better flag name? "disableEvals" ? "strictMode" ( could be an umbrella flag, also prohibits cleartext plugin etc ). disableGenFunction
( again, too focused on implementation ). Naming things is hard 🤷♂️
re testing - thanks a lot for dev documentation. Are you aware of any way to test workers locally or in actions runtime? something similar to localstack. Ideally with the same checks, e.i errors when code tries to eval. Any container we can use for that?
Are you aware of any way to test workers locally or in actions runtime?
Cloudflare Workers provide an unstable_dev
API for E2E / integration tests:
https://developers.cloudflare.com/workers/wrangler/api/#unstable_dev
Test code in node-postgres:
https://github.com/brianc/node-postgres/blob/6cd0aeb212d1672edd33499b2f4f858cf7ed9a79/packages/pg/test/worker/src/index.test.js#L13
I can probably merge it first, together with added binary protocol non-jit parser. I don't like the flag name "useStaticParser", it reflects more implementation details rather than intention, but don't have better ideas. Can you think of a better flag name? "disableEvals" ? "strictMode" ( could be an umbrella flag, also prohibits cleartext plugin etc ).
disableGenFunction
( again, too focused on implementation ). Naming things is hard 🤷♂️
I prefer disableEvals
among these names. You can also use other names, not good at naming for me :(
I prefer
disableEvals
among these names. You can also use other names, not good at naming for me :(
I like it more than useStaticParser
, I think I'll update in my branch ( together with a version for no evals binary parser ) and comment here when ready
I have tested this pr on multiple MySQL server ranging from old 5.7 to more recent MySQL and MariaDB version locally and on deployed workers so far it works flawlessly. The only issue is having to set node_compat = true
instead of compatibility_flags = [ "nodejs_compat" ]
but that's a small price to pay.
Also it's currently not possible to use drizzle with msyql driver but kysely works ok.
FYI:
- Other drivers (like
node-postgres
andPostgres.js
) also requirenode_compat = true
today - We have plans to allow
nodejs_compat
to work: this will likely require us to better audit specific packages and conditionally load/exclude Node.js APIs that we don't have support for in Workers, or that don't make sense in a serverless environment like Workers.
Otherwise, we'd love to see this merged + we're bringing MySQL support to Hyperdrive: https://developers.cloudflare.com/hyperdrive/
I have resolved the conflict with master branch. @sidorares Hi, what's the process of no evals parser? I'd like to remove the related codes in the PR after that.
Hi, @shiyuhang0 🙋🏻♂️
If it's okay, I'd like to make a few simple suggestions in the documentation:
- Change from
.md
to.mdx
- Change the title from Run on Cloudflare Workers to Cloudflare Workers
- I think it fits in the Examples section, what do you think?
Hi, @shiyuhang0 🙋🏻♂️
If it's okay, I'd like to make a few simple suggestions in the documentation:
- Change from
.md
to.mdx
- Change the title from Run on Cloudflare Workers to Cloudflare Workers
- I think it fits in the Examples section, what do you think?
already doing this, will push a commit soon
@shiyuhang0 what do you think about useStaticParser
flag name? Should we rename it to disableEval
before its too late and somebody else depends on this name?
@shiyuhang0 what do you think about
useStaticParser
flag name? Should we rename it todisableEval
before its too late and somebody else depends on this name?
Both it is fine for me. But we shouldn't worry about modifying parameter names in an unmerged PR. For me, I think it can even be modified before release.
I will adjust this PR after the name is determined
I really need this feature. I need to use the node-mysql2 driver with Drizzle in Cloudflare Workers. If everything is confirmed to be correct, could you please merge this PR as soon as possible? Thanks
@sidorares
@m430 last time I checked Drizzle/Mysql was not compatible with cloudflare even using this updated driver. That is because of some node compatibility issue. If you really need a mysql driver for cf today! you can use a git dependecy or @nora-soderlund cloudflare-mysql
@shiyuhang0 disableEval makes more sense to me
@shiyuhang0 missed your message, I also prefer disableEval
. Are you able to continue or you need someone to take over your work?
@shiyuhang0 missed your message, I also prefer
disableEval
. Are you able to continue or you need someone to take over your work?
@sidorares I have pushed a new commit to use disableEval
instead of useStaticParser
. Is there anything else I need to do?
Codecov Report
Attention: Patch coverage is 55.14706%
with 183 lines
in your changes are missing coverage. Please review.
:exclamation: No coverage uploaded for pull request base (
master@41d21eb
). Click here to learn what that means.
Additional details and impacted files
@@ Coverage Diff @@
## master #2289 +/- ##
=========================================
Coverage ? 89.47%
=========================================
Files ? 76
Lines ? 16021
Branches ? 1363
=========================================
Hits ? 14334
Misses ? 1687
Partials ? 0
Flag | Coverage Δ | |
---|---|---|
compression-0 | 89.47% <55.14%> (?) |
|
compression-1 | 89.47% <55.14%> (?) |
|
tls-0 | 88.96% <52.45%> (?) |
|
tls-1 | 89.08% <46.07%> (?) |
Flags with carried forward coverage won't be shown. Click here to find out more.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Add node_compat = true flag to polyfill the missing node modules
Is there a path to instead supporting our modern nodejs_compat
mode by updating imports (conditionally) vs using the legacy polyfill mode?
https://developers.cloudflare.com/workers/runtime-apis/nodejs/
I have pushed a new commit to use
disableEval
instead ofuseStaticParser
. Is there anything else I need to do?
Just a note, while in the docs it says disableEval
, in code it's useStaticParser
🙋🏻♂️
I have pushed a new commit to use
disableEval
instead ofuseStaticParser
. Is there anything else I need to do?Just a note, while in the docs it says
disableEval
, in code it'suseStaticParser
🙋🏻♂️
I forget push the change in code