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

refactor: do not use deno_fs::FileSystem everywhere #27508

Merged
merged 5 commits into from
Dec 31, 2024

Conversation

dsherret
Copy link
Member

@dsherret dsherret commented Dec 31, 2024

This changes the cli to mostly use std::fs via sys_traits instead of the implemention of deno_fs::FileSystem.

This has two benefits:

  1. It's going to be easier to extract out code from the CLI create because it no longer depends on the deno_fs crate.
  2. Should be slightly more performant because it's not going through the deno_fs file system abstraction anymore, which does a lot of extra work for things like stat and reading directories.

Closes #27464

@dsherret dsherret marked this pull request as ready for review December 31, 2024 01:54
Copy link
Member Author

@dsherret dsherret Dec 31, 2024

Choose a reason for hiding this comment

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

This is so verbose because we don't have conditional compilation for denort (see the todo on line 3). We can improve this over time.

Copy link
Member Author

Choose a reason for hiding this comment

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

sys_traits offers an in-memory system, so we can just delete this now.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd like to switch the FileSystem trait back to a type parameter, but first we should split up deno and denort so they're compiled separately.

Copy link
Member

Choose a reason for hiding this comment

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

Let's discuss this offline

@dsherret dsherret requested a review from bartlomieju December 31, 2024 01:59
@@ -1,12 +1,13 @@
// Copyright 2018-2024 the Deno authors. All rights reserved. MIT license.

use crate::sys::CliSys;
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this one be under all the other imports? 😏

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's merge that other fmt pr 😅

I'll rebase it in a few minutes.

Copy link
Member

Choose a reason for hiding this comment

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

Let's discuss this offline

Comment on lines -419 to -424
// todo(dsherret): this is temporary. Instead of using the `FileSystem` trait implementation
// in the CLI, the CLI should instead create it's own file system using `sys_traits` traits
// then that can implement the `FileSystem` trait. Then this `FileSystem` trait can stay here
// for use only for `ext/fs` and not the entire CLI.
#[derive(Debug, Clone)]
pub struct FsSysTraitsAdapter(pub FileSystemRc);
Copy link
Member

Choose a reason for hiding this comment

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

Looks like deno_core is not upgraded in this PR, not needed anymore?

@dsherret dsherret merged commit 4638caa into denoland:main Dec 31, 2024
20 checks passed
@dsherret dsherret deleted the invert_file_system_trait branch December 31, 2024 16:29
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