-
Notifications
You must be signed in to change notification settings - Fork 5.8k
refactor: remove permission traits + generics from extension crates #31284
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: remove permission traits + generics from extension crates #31284
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the Deno codebase by removing generic permission traits from extension crates and consolidating permission handling. The RuntimePermissionDescriptorParser struct is moved from deno_runtime into deno_permissions to enable extension crates to use it in tests.
Key changes:
- Removed generic type parameters for permissions from all extension crates (deno_web, deno_fetch, deno_net, deno_fs, deno_ffi, deno_napi, deno_node, deno_websocket)
- Replaced trait-based permission abstractions with direct usage of
PermissionsContainer - Moved
RuntimePermissionDescriptorParserfrom runtime/permissions.rs to runtime/permissions/runtime_descriptor_parser.rs and re-exported it
Reviewed Changes
Copilot reviewed 38 out of 39 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| runtime/worker.rs | Removed generic permission parameters from extension initialization |
| runtime/web_worker.rs | Removed generic permission parameters from extension initialization |
| runtime/snapshot_info.rs | Removed unused permission trait implementations for snapshot creation |
| runtime/snapshot.rs | Removed generic permission parameters from snapshot extensions |
| runtime/permissions/runtime_descriptor_parser.rs | New file containing moved RuntimePermissionDescriptorParser implementation |
| runtime/permissions/lib.rs | Added export for RuntimePermissionDescriptorParser |
| runtime/permissions.rs | Reduced to single re-export line |
| ext/websocket/lib.rs | Removed generic parameters and used PermissionsContainer directly |
| ext/web/timers.rs | Removed TimersPermission trait and always allow high-resolution time |
| ext/web/lib.rs | Removed generic permission parameter |
| ext/web/benches/* | Updated tests to remove custom permission implementations |
| ext/node/ops/* | Removed generic permission parameters from ops |
| ext/node/lib.rs | Removed permission generic parameter from extension |
| ext/net/* | Removed NetPermissions trait and used PermissionsContainer directly |
| ext/napi/lib.rs | Removed NapiPermissions trait |
| ext/fs/* | Removed FsPermissions trait |
| ext/ffi/* | Removed FfiPermissions trait |
| ext/fetch/lib.rs | Removed FetchPermissions trait |
| ext/net/Cargo.toml | Added sys_traits dev-dependency for tests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cc199b6 to
329d9fa
Compare
| pub fn ensure_read_permission<'a>( | ||
| &self, | ||
| permissions: &mut dyn NodePermissions, | ||
| permissions: &mut PermissionsContainer, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these traits are necessary for deploy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh maybe not. I remember now that a bunch of code was using PermissionsContainer anyway
dsherret
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
littledivy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
I also moved the
RuntimePermissionDescriptorParserstruct from deno_runtime into deno_permissions, so that extension crates can use it in tests and stuff like that