Conversation
There was a problem hiding this comment.
Pull request overview
This is a work-in-progress pull request updating RustyScript from an older Deno version to Deno 2.6.0. The update involves significant API changes, particularly around permissions handling, V8 scope management, and module loading.
Key Changes:
- Upgraded V8 API from
HandleScopetoPinScopethroughout the codebase - Updated module loader API to use consolidated parameters (
ModuleLoadOptionsandModuleLoadReferrer) - Removed separate
deno_consoleanddeno_urlextensions, integrating console functionality intoweb_stub
Reviewed changes
Copilot reviewed 34 out of 36 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/traits.rs |
Updated V8 scope types from HandleScope to PinScope |
src/runtime.rs |
Migrated to new scope! macro for V8 scope management |
src/op_whitelist.rs |
Removed deno_console and deno_url op whitelists |
src/module_loader/*.rs |
Updated to new module loading API with consolidated options |
src/inner_runtime.rs |
Updated scope types and error handling |
src/js_value/*.rs |
Migrated all V8 interactions to PinScope |
src/ext/web_stub/*.rs |
Added console functionality, replacing deno_console extension |
src/ext/web/permissions.rs |
Updated permission error structure with new required fields |
src/error.rs |
Changed JsError handling to use Box directly |
Cargo.toml |
Updated all deno dependencies to 2.6.0 compatible versions |
readme.md |
Updated documentation reflecting removed extensions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fn decode_args<'a>( | ||
| args: &impl serde::ser::Serialize, | ||
| scope: &mut v8::HandleScope<'a>, | ||
| scope: &mut v8::PinnedRef<'a, v8::TryCatch<'_, '_, v8::HandleScope<'_>>>, |
There was a problem hiding this comment.
The scope parameter type has changed from &mut v8::HandleScope<'_> to &mut v8::PinnedRef<'_, v8::HandleScope<'_>>, but the function body is unchanged. This may cause compilation issues since PinnedRef requires dereferencing to access the underlying HandleScope for TryCatch creation.
|
|
||
| use super::ExtensionTrait; | ||
|
|
||
| mod console; |
There was a problem hiding this comment.
A new console module is added to web_stub. Consider adding documentation explaining why console functionality was moved from a separate extension into the web_stub extension, as this is a significant architectural change.
| custom_message: None, | ||
| state: deno_permissions::PermissionState::Denied |
There was a problem hiding this comment.
Two new required fields are added to PermissionDeniedError: custom_message and state. The PR description mentions incomplete permission handling work. Verify that PermissionState::Denied is always the correct state for this error context, and consider whether custom_message should be configurable rather than always None.
| @@ -123,7 +123,7 @@ impl ExtensionTrait<WebOptions> for init_web { | |||
|
|
|||
| impl ExtensionTrait<WebOptions> for deno_web::deno_web { | |||
| fn init(options: WebOptions) -> Extension { | |||
There was a problem hiding this comment.
A Default::default() parameter is added to the deno_web::init call without explanation. Review the deno_web API documentation to understand what this parameter represents and whether a custom value should be used instead of the default.
| fn init(options: WebOptions) -> Extension { | |
| fn init(options: WebOptions) -> Extension { | |
| // The third parameter to `deno_web::deno_web::init` is of type `deno_web::deno_web::Options`. | |
| // Using `Default::default()` here is appropriate because no custom web options are needed in this context. |
| let local = self.0.as_local(&scope); | ||
| Ok(deno_core::serde_v8::from_v8(scope, local)?) |
There was a problem hiding this comment.
The scope variable is borrowed mutably but passed immutably with &scope instead of scope. This inconsistency may cause issues - either remove the reference or pass it as mutable if required by the API.
| let local = self.0.as_local(&scope); | |
| Ok(deno_core::serde_v8::from_v8(scope, local)?) | |
| let local = self.0.as_local(&mut scope); | |
| Ok(deno_core::serde_v8::from_v8(&mut scope, local)?) |
| ArrayPrototypePush(rawEntries, [currentKey, value]); | ||
| } | ||
| currentKey = null; | ||
| currentPart = ""; |
There was a problem hiding this comment.
The value assigned to currentPart here is unused.
| currentPart = ""; |
1 participant
Deno changed the way they are handling permissions, I dont have time to finalize it for now.