fix(🧪): add support for explicit resource management #3394
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for explicit resource management in the Skia package by implementing the Symbol.dispose pattern and updating the TypeScript configuration to ES2024. The changes enable automatic resource cleanup using the using keyword instead of manual dispose() calls.
- Updates TypeScript configuration to ES2024 with explicit resource management support
- Implements
Symbol.disposepattern across Skia classes and refactors dispose methods - Updates test files to use the
usingkeyword for automatic resource management
Reviewed Changes
Copilot reviewed 56 out of 59 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/skia/tsconfig.json | Updates TypeScript target to ES2024 and adds ES2024 lib support |
| packages/skia/src/skia/web/*.ts | Implements Symbol.dispose pattern and removes standalone dispose methods |
| packages/skia/src/skia/web/Host.ts | Adds default Symbol.dispose implementation in BaseHostObject |
| packages/skia/src/skia/types/JsiInstance.ts | Extends interface with Disposable |
| test files | Updates tests to use using keyword for automatic resource management |
| configuration files | Adds Babel plugin and updates dependencies for explicit resource management |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| getLineMetrics(): LineMetrics[] { | ||
| return this.ref.getLineMetrics(); | ||
| } | ||
|
|
There was a problem hiding this comment.
The dispose() method calls this[Symbol.dispose]() but there's no [Symbol.dispose]() method defined in this class. This will result in a runtime error when dispose() is called.
| [Symbol.dispose]() { | |
| // Implement cleanup logic here if needed. | |
| // For now, this is a no-op to prevent runtime errors. | |
| } |
Copilot uses AI. Check for mistakes.
| constructor(CanvasKit: CanvasKit, ref: FontMgr) { | ||
| super(CanvasKit, ref, "FontMgr"); | ||
| } | ||
|
|
There was a problem hiding this comment.
The dispose() method calls this[Symbol.dispose]() but there's no [Symbol.dispose]() method defined in this class. This will result in a runtime error when dispose() is called.
| [Symbol.dispose]() { | |
| // No-op or add cleanup logic here if needed | |
| } |
Copilot uses AI. Check for mistakes.
| // Check for dispose symbol as last resort | ||
| static const auto disposeSymbol = jsi::PropNameID::forSymbol( | ||
| runtime, | ||
| eval(runtime, "Symbol.for('Symbol.dispose');").getSymbol(runtime)); |
There was a problem hiding this comment.
Using Symbol.for('Symbol.dispose') instead of the actual Symbol.dispose symbol. The correct symbol should be accessed via Symbol.dispose, not Symbol.for('Symbol.dispose').
| eval(runtime, "Symbol.for('Symbol.dispose');").getSymbol(runtime)); | |
| eval(runtime, "Symbol.dispose").getSymbol(runtime)); |
Copilot uses AI. Check for mistakes.
| */ | ||
| fromBytes(bytes: Uint8Array) { | ||
| return new JsiSkData(this.CanvasKit, bytes); | ||
| return new JsiSkData(this.CanvasKit, bytes as unknown as ArrayBuffer); |
There was a problem hiding this comment.
The double type assertion as unknown as ArrayBuffer suggests a type mismatch. Consider fixing the underlying type issue instead of using a double assertion, which bypasses TypeScript's type safety.
| return new JsiSkData(this.CanvasKit, bytes as unknown as ArrayBuffer); | |
| // Ensure we pass only the relevant ArrayBuffer portion | |
| const arrayBuffer = bytes.buffer.slice(bytes.byteOffset, bytes.byteOffset + bytes.byteLength); | |
| return new JsiSkData(this.CanvasKit, arrayBuffer); |
Copilot uses AI. Check for mistakes.
|
🎉 This PR is included in version 2.2.19 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
No description provided.