-
Notifications
You must be signed in to change notification settings - Fork 65
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use vscode
instead of fs
to create new files
#161
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just getting back to reviewing activity on this repo after long hiatus....
This looks like a good change, but I am a little concerned about the rest of the downstream effects of switching from sync to async. For example, as I think I understand it, createNewNoteFile
can now return before the file is actually created, correct?
Have you investigated all the possible downstream effects of this and verified this doesn't introduce an edge case / race condition that might cause an error?
Also, can you tell me a little bit about how you tested this?
Thanks!
Hey @kortina, sorry for the delay! I made this PR for a project I was working on, but have since abandoned. Correct, The only way I could see a race condition happening is by clicking the
Seems pretty safe. What outcomes are you worried about? |
OK, ya, my main concern was that I almost always use And the only way you triggered the failure mode is if you somehow trigger the create+open function 2x? Please confirm, and if that's the case I will checkout and play with the branch myself. Thanks! |
src/NoteWorkspace.ts
Outdated
const edit = new vscode.WorkspaceEdit(); | ||
const fileUri = vscode.Uri.file(filepath); | ||
edit.createFile(fileUri); | ||
vscode.workspace.applyEdit(edit).then(() => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see here there is opportunity for a callback. I was thinking I wonder if there's a way to somehow leverage this when using goto definition for a non-existent note, ie here:
In the case where we must create the note, can we await the async vscode create file operation before we return the vscode.Location array in this function?
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, makes sense to use async. Changed
I'm on a Mac so had to look up the keybinding. Seems like that
I made another test where I changed the permission of the folder to not allow writes (555), and this will just not create the file, and will not throw an error. Tried your extension and it behaved the same. I could not trigger the error. Clicking the |
Summary
Use
vscode.WorkspaceEdit.createFile
instead offs.writeFileSync
to create new files.Why
Other extensions on vscode might want to know when files are created in the workspace. The best way to do this, is by listening for a
FileCreateEvent
withvscode.workspace.onDidCreateFiles
. This event is not fired ifvscode.workspace.fs
or a foreign program or script creates the file.Choices made
Since
vscode.workspace.fs.writeFile
will also create a file if none is found, we wait for thevscode.workspace.applyEdit
to finish to execute it.