refactor: use sys_traits #27480
refactor: use sys_traits #27480
Conversation
| @@ -91,8 +92,9 @@ impl CliLockfile { | |||
| // do an atomic write to reduce the chance of multiple deno | |||
| // processes corrupting the file | |||
| atomic_write_file_with_retries( | |||
| &FsSysTraitsAdapter::new_real(), | |||
There was a problem hiding this comment.
This FsSysTraitsAdapter is an adapter for the traits in sys_traits and deno_fs::FileSystem. It's temporary though. After this PR, I'll decouple the CLI crate's code from deno_fs as that makes extracting code from the CLI crate a lot easier.
| @@ -8,62 +8,6 @@ use deno_semver::jsr::JsrDepPackageReq; | |||
| use deno_semver::jsr::JsrPackageReqReference; | |||
| use deno_semver::npm::NpmPackageReqReference; | |||
|
|
|||
| #[cfg(test)] // happens to only be used by the tests at the moment | |||
| pub struct DenoConfigFsAdapter<'a>( | |||
There was a problem hiding this comment.
The benefit of sys_traits is that we can remove all these adapter and fs implementation structs. It makes setup code a lot easier for people using our crates.
|
Is there an impact on binary size? |
|
Doubt it. Maybe a very tiny reduction. |
| @@ -1274,7 +1275,7 @@ dependencies = [ | |||
| "deno_npm", | |||
| "deno_npm_cache", | |||
| "deno_package_json", | |||
| "deno_path_util", | |||
| "deno_path_util 0.3.0", | |||
There was a problem hiding this comment.
But I think this is ok to land without because this is a really small dependency -- Will update this pr with that in a bit.
There was a problem hiding this comment.
There was actually too big of a change in deno_doc for me to do the upgrade. We'll have to do that later.
| @@ -867,6 +867,7 @@ impl CliOptions { | |||
| ConfigFlag::Discover => { | |||
| if let Some(start_paths) = flags.config_path_args(&initial_cwd) { | |||
| WorkspaceDirectory::discover( | |||
| &FsSysTraitsAdapter::new_real(), | |||
There was a problem hiding this comment.
I hope creating these is rather cheap
| @@ -79,6 +49,37 @@ use node_resolver::errors::ClosestPkgJsonError; | |||
| use node_resolver::InNpmPackageChecker; | |||
| use node_resolver::NodeResolutionKind; | |||
| use node_resolver::ResolutionMode; | |||
| use sys_traits::FsRead; | |||
|
|
|||
| use crate::args::jsr_url; | |||
There was a problem hiding this comment.
Is there a reason to shift all of these in various files?
| @@ -25,10 +25,12 @@ deno_io.workspace = true | |||
| deno_path_util.workspace = true | |||
| deno_permissions.workspace = true | |||
| filetime.workspace = true | |||
| getrandom = "0.2" | |||
There was a problem hiding this comment.
Is this used on purpose instead of rand crate?
There was a problem hiding this comment.
Yes, this is what the rand crate uses and the SystemRandom trait needs this lower level function
| // 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)] |
There was a problem hiding this comment.
Do you have a follow up PR prepared or is this a TODO for some other time in the future? If the latter, consider opening an issue for it
There was a problem hiding this comment.
I have a PR incoming after this one.
A follow-up to this will be decoupling the CLI from ext/fs's FileSystem trait.