Fix deprecated APIs warnings #259
Conversation
|
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 👀 |
It will be stabilized in Deno v1.44
|
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. |
|
Tests pass now on Deno v1.44! Let me know if you have any suggestions or feedback on this PR. |
build/vfs.js
Outdated
| }, | ||
| // Acquire a SHARED or EXCLUSIVE file lock | ||
| js_lock: (rid, exclusive) => { | ||
| // this is unstable and has issues on Windows ... | ||
| if (Deno.flockSync && !isWindows) Deno.flockSync(rid, exclusive !== 0); | ||
| if (Deno.flockSync && !isWindows) { |
There was a problem hiding this comment.
@dyedgreen What do you think is the best way to detect the availability of FsFile.lockSync?
FsFile.lockSyncis available in v1.44.0, butDeno.flockSyncis still undefined without--unstableor--unstable-fs. So we can't checkDeno.flockSync.FsFile.lockSyncexists in versions before v1.44.0, but calling it terminates deno, even from inside of a try/catch block. So we can't check the existence ofFsFile.lockSync.Deno.version.deno >= "1.44.0"is the only condition I could get to work. Should we go with that?
There was a problem hiding this comment.
Yeah I feel like we’ll just need to do a version check here 😕
| }, | ||
| // Sync file data to disk | ||
| js_sync: (rid) => { | ||
| Deno.fdatasyncSync(rid); | ||
| const file = getOpenFile(rid); | ||
| file.syncDataSync(); |
There was a problem hiding this comment.
@dyedgreen I don't know of a good way to make this method support Deno versions before v1.44.
- We could make it a no-op like the lock/unlock method. Is that acceptable or would that break things?
- Or we could bump the minimum supported Deno version to v1.44 after this lands.
What do you think?
There was a problem hiding this comment.
@dyedgreen Let me know when you get a chance to look at this. I'm thinking we may need to bump the minimum supported Deno version to get access to the new file flushing APIs. What are your thoughts?
|
@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! |
|
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! |
|
@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. |
We now require at least Deno v1.44 for stable syncDataSync. lockSync/unlockSync were also stabilized in Deno v1.44 so there is no reason to check for their availability anymore.
|
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 |
Prepare for Deno 2.0 by migrating off of deprecated APIs. Fixes #255.
Major CaveatWithDENO_FUTURE=1set, there is no stable sync data operation available as of Deno v1.42. EnablingDENO_FUTUREremoves access to theDeno.fdatasyncSyncmethod, andDeno.FsFile.syncDataSyncis still unstable. WithDENO_FUTURE=1enabled, 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 forDeno.FsFile.syncDataSyncto 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!