search

dyedgreen / deno-sqlite

Deno SQLite module
https://deno.land/x/sqlite
MIT License
409 stars 36 forks source link

Fix deprecated APIs warnings #259

Closed canac closed 1 month ago

canac commented 6 months ago

Prepare for Deno 2.0 by migrating off of deprecated APIs. Fixes #255.

Major Caveat

With DENO_FUTURE=1 set, there is no stable sync data operation available as of Deno v1.42. Enabling DENO_FUTURE removes access to the Deno.fdatasyncSync method, and Deno.FsFile.syncDataSync is still unstable. With DENO_FUTURE=1 enabled, this library will only run with --unstable-fs (or --unstable).

Please let me know if you want me to document this caveat in the README or if you'd rather wait for Deno.FsFile.syncDataSync to stabilize before merging.

Edit: this limitation was resolved in Deno v1.44. This branch now successfully runs with DENO_FUTURE=1.

Thanks for creating this library! It's really easy to use in my scripts. I'm glad to have a chance to contribute back a little bit!

dyedgreen commented 6 months ago

Hello, thanks for the contribution!

This mostly looks good code wise, but requiring unstable to run is not great. I’ll see what the status of these APIs is on the deno side 👀

canac commented 5 months ago

I pushed a new commit that uses the not-yet stabilized APIs. The tests are failing now, but they should pass if we rerun them after Deno v1.44 releases.

canac commented 4 months ago

Tests pass now on Deno v1.44! Let me know if you have any suggestions or feedback on this PR.

canac commented 1 month ago

@dyedgreen With Deno 2.0 supposedly right around the corner, it would be awesome to push this PR across the finish line. My one remaining question is whether bumping up the minimum support Deno version to v1.44 is acceptable. Cheers!

dyedgreen commented 1 month ago

Hey! This mostly looks good. One small thing I'd want to change is use an array instead of a map to store the open files; I made some changes locally but I can't seem to push them to your branch -- can you take a look at making that change?

Otherwise this LGTM!

canac commented 1 month ago

@dyedgreen I checked the box, but sorry it won't let you push to the branch. Thanks for looking at it again! I'll try to make that change this week.

canac commented 1 month ago

I tested this branch with a bunch of Deno versions, and it works in Deno pre-v1.44 as long as you pass in the --unstable-fs flag.