Skip to content

chore(fs): document the deprecation of exists and existsSync #2730

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

Merged
merged 4 commits into from
Oct 3, 2022
Merged

chore(fs): document the deprecation of exists and existsSync #2730

merged 4 commits into from
Oct 3, 2022

Conversation

iuioiua
Copy link
Contributor

@iuioiua iuioiua commented Sep 30, 2022

exists was deprecated because it causes a race condition when used before a file operation. However, it still has some legitimate uses cases, even within std. The community has expressed more arguments here:

This PR:

  1. Reverts the deprecation of exists and existsSync.
  2. Advises against using exists before a file operation and clearly explains the reasoning.
  3. Closes docs: give more detailed description/reasoning/alternative suggestions for the deprecation of fs.exists #2125.
  4. Cleans up an illegitimate use of exists within the codebase.
  5. Adds URL support for the argument.

A few legitimate use cases were also likely removed in #2546, as Deno.lstat is still used to check for the existence of files. It'd be a good idea to revisit these removals and consider re-adding the exists, but only where appropriate.

@iuioiua iuioiua marked this pull request as ready for review September 30, 2022 21:28
@kt3k
Copy link
Member

kt3k commented Oct 2, 2022

The update of the document looks good to me. Also the removal of the usage in log is very nice!

But let's keep them deprecated for now because the core team is still in favor of deprecation.

Also in my view, the community is also not totally on the same page on reverting deprecation. Some people still prefer the deprecation, and there are suggestion of modified version of API instead of exists/existsSync. See the discussion in https://github.com/denoland/deno_std/discussions/2102 for details. I think we need more discussion before making decision on this

@iuioiua
Copy link
Contributor Author

iuioiua commented Oct 2, 2022

Ok, I can reduce the scope of this PR by ignoring the actual deprecation of it for now.

Please check my previous comment about the use of exists within this repo in std/log. How do we cater for that use case without using exists?

@iuioiua iuioiua changed the title chore(fs): undo deprecation of exists and existsSync chore(fs): document the deprecation of exists and existsSync Oct 3, 2022
@kt3k
Copy link
Member

kt3k commented Oct 3, 2022

Thanks for updating!

However, it still has some legitimate uses cases, even within std.

Ok. This looks more legitimate usage than typical ones. I also observe several usages in our testing like assertEquals(existsSync(srcDir), true);. I'll bring this up in core team meetings.

Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kt3k kt3k merged commit 12912dd into denoland:main Oct 3, 2022
@iuioiua iuioiua deleted the exists branch October 3, 2022 09:08
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.

docs: give more detailed description/reasoning/alternative suggestions for the deprecation of fs.exists
2 participants