Skip to content

feat: writeFileAtomic module#142

Open
muskaanshraogi wants to merge 3 commits into
npm:mainfrom
muskaanshraogi:feature/write-file-atomic
Open

feat: writeFileAtomic module#142
muskaanshraogi wants to merge 3 commits into
npm:mainfrom
muskaanshraogi:feature/write-file-atomic

Conversation

@muskaanshraogi
Copy link
Copy Markdown

Adds a crash-safe writeFileAtomic helper using temp file + rename pattern. Prevents partial writes in scenarios like process crashes or interruptions.

References

Related to #55

@wraithgar
Copy link
Copy Markdown
Contributor

Is this pulling in https://github.com/npm/write-file-atomic or is this a new novel implementation?

@muskaanshraogi
Copy link
Copy Markdown
Author

@wraithgar if by pulling you mean directly use, then no it is not. But the implementation is based on the same logic. I write to a new temp file, and then replace. Related to #55

@muskaanshraogi
Copy link
Copy Markdown
Author

Hi @wraithgar, just checking if anyone has bandwidth to review this. Happy to make changes

@wraithgar
Copy link
Copy Markdown
Contributor

I think the idea was to bring in write-file-atomic since that is a package we maintain ourselves already.

@muskaanshraogi
Copy link
Copy Markdown
Author

@wraithgar understood. I have updated my PR. this time i'm pulling in write-file-atomic as is. Let me know if you have any suggestions. I assume the README will have to be updated too?

@wraithgar
Copy link
Copy Markdown
Contributor

I think the idea is that we add it as a dependency. There is no need to duplicate it, it already exists as a package.

@muskaanshraogi
Copy link
Copy Markdown
Author

@wraithgar let me make sure I understand what you're saying. Are you suggesting to add write-file-atomic as a dependency, import the module, and then export it? If yes, then should the imported module first be wrapped? what functionality is the wrapper expected to add?

My current understanding of shifting the entire implementation comes from the open issue #55 which states that the functionality should be implemented here. Also I took reference of a similar issue #56 . This was about move-file, and it was closed after shifting the entire logic here with this PR. Subsequently, the move-file repository was archived.

@owlstronaut
Copy link
Copy Markdown
Contributor

@muskaanshraogi sorry for the slow response here

yes, add write-file-atomic as a runtime dep and re-export it from lib/index.js, no wrapper needed. the move-file was the right call for that package because the upstream was a tiny unmaintained fork we'd diverged from.

So reworking this would be:

  • drop the signal-exit package (transitive to write-file-atomic)
  • add the 'write-file-atomic' package in an atomic commit
  • include write-file-atomic in the module.exports
  • remove the copy-paste of write-file-atomic
  • add a writeFIleAtomic section to the readme

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants