Skip to content
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

fix: fixed minor bug with utils/file exists() function #683

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ahmedosama94
Copy link

@ahmedosama94 ahmedosama94 commented Jan 13, 2025

Description

While investigating an issue I had with the JSON loader, I found that the exists function does not work as intended. The node:fs/promises stat() function resolves with the stats data if the file exists, but the promise is rejected when the file does not exist, so the function never really returns false, swallowing the errors for getFiles in the parseTranslations() function as well as the new I18nError at the top of parseTranslations() after the exists() check.

I also came across this line

Using fs.stat() to check for the existence of a file before calling fs.open(), fs.readFile(), or fs.writeFile() is not recommended. Instead, user code should open/read/write the file directly and handle the error raised if the file is not available. To check if a file exists without manipulating it afterwards, fs.access() is recommended.
In the Node.JS docs here: https://nodejs.org/api/fs.html#fsstatpath-options-callback

So I also switched the implementation to use fs.access instead of fs.stat.

Linked Issues

Additional context

Quick view of the simple tests that I added:

image

and with the original implementation:
image

here are the test results:
image

and with the updated implementation:
image

here are the test results:
image

@coveralls
Copy link

Coverage Status

coverage: 90.853% (+0.02%) from 90.829%
when pulling 7eb0864 on ahmedosama94:util-file-exists-fix
into 1efa63d on toonvanstrijp:main.

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.

2 participants