Add fast invoke JS runtime methods to Butil (#9729)#9731
Add fast invoke JS runtime methods to Butil (#9729)#9731msynk merged 3 commits intobitfoundation:developfrom
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe pull request introduces a comprehensive update to the JavaScript interoperability methods across multiple classes in the Bit.Butil library. The primary change involves replacing standard Changes
Sequence DiagramsequenceDiagram
participant DotNet as .NET Client
participant JSRuntime as IJSRuntime
participant JSMethod as JavaScript Method
DotNet->>JSRuntime: FastInvokeAsync/FastInvokeVoidAsync
JSRuntime->>JSMethod: Invoke JavaScript method
JSMethod-->>JSRuntime: Return result
JSRuntime-->>DotNet: Return result
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (11)
src/Butil/Bit.Butil/Publics/Storage/ButilStorage.cs (1)
20-20: Consider adding retry mechanism for storage operations.While the switch to
FastInvoke*methods may improve performance, storage operations often benefit from retry mechanisms, especially in unreliable network conditions or when storage quotas are reached.Consider wrapping the storage operations with a retry mechanism:
public async Task<string?> GetItem(string? key) - => await js.FastInvokeAsync<string?>("BitButil.storage.getItem", storageName, key); + => await WithRetry(() => js.FastInvokeAsync<string?>("BitButil.storage.getItem", storageName, key)); +private async Task<T> WithRetry<T>(Func<Task<T>> operation, int maxRetries = 3) +{ + for (int i = 0; i < maxRetries; i++) + { + try + { + return await operation(); + } + catch (Exception) when (i < maxRetries - 1) + { + await Task.Delay((i + 1) * 100); // Exponential backoff + } + } + return await operation(); // Let the last attempt throw +}Also applies to: 28-28, 36-36, 44-44, 52-52, 60-60
src/Butil/Bit.Butil/Publics/ScreenOrientation.cs (2)
26-26: LGTM! Consider using string constants for type mapping.The changes are well-implemented. For better type safety and maintainability, consider extracting the orientation type strings into constants.
+private static class OrientationTypes +{ + public const string Any = "any"; + public const string Natural = "natural"; + public const string Landscape = "landscape"; + public const string Portrait = "portrait"; + public const string PortraitPrimary = "portrait-primary"; + public const string PortraitSecondary = "portrait-secondary"; + public const string LandscapePrimary = "landscape-primary"; + public const string LandscapeSecondary = "landscape-secondary"; +} public async Task Lock(OrientationLockType lockType) { var type = lockType switch { - OrientationLockType.Any => "any", - OrientationLockType.Natural => "natural", - OrientationLockType.Landscape => "landscape", + OrientationLockType.Any => OrientationTypes.Any, + OrientationLockType.Natural => OrientationTypes.Natural, + OrientationLockType.Landscape => OrientationTypes.Landscape, // ... similar changes for other cases - _ => "any" + _ => OrientationTypes.Any };Also applies to: 44-44, 67-67, 76-76, 90-90, 152-152
Line range hint
1-1: Consider documenting performance implications of FastInvoke methods.The systematic replacement of
Invoke*withFastInvoke*methods appears to be a performance optimization. Consider:
- Documenting the expected performance benefits
- Adding monitoring/metrics to track the impact
- Creating a migration guide for other teams
src/Butil/Bit.Butil/Extensions/JSRuntimeExtensions.cs (3)
28-59: Consider enhancing error handling for better observability.The error handling for JSON serialization issues is good, but logging to console might not be sufficient in production environments.
Consider enhancing the error handling:
catch (JsonException ex) { - System.Console.WriteLine($"Error invoking '{identifier}' using {nameof(IJSInProcessRuntime)}. A JSON-related issue occurred: {ex.Message}."); + // Use a proper logging framework + System.Console.WriteLine($"[Error] Failed to invoke '{identifier}' using {nameof(IJSInProcessRuntime)}. JSON error: {ex.Message}. Stack trace: {ex.StackTrace}"); + // Consider adding telemetry or metrics for monitoring JSON serialization failures return ValueTask.FromResult(default(TResult)!); }
61-102: Consider adding retry logic for transient failures.The void async methods could benefit from retry logic for transient failures, especially in the out-of-process scenario.
Consider adding retry logic:
else { - return jsRuntime.InvokeVoidAsync(identifier, cancellationToken, args); + // Add retry logic for transient failures + const int maxRetries = 3; + const int delayMilliseconds = 100; + + for (int i = 0; i < maxRetries; i++) + { + try + { + return await jsRuntime.InvokeVoidAsync(identifier, cancellationToken, args); + } + catch (Exception ex) when (IsTransientException(ex) && i < maxRetries - 1) + { + await Task.Delay(delayMilliseconds * (i + 1), cancellationToken); + } + } + + return jsRuntime.InvokeVoidAsync(identifier, cancellationToken, args); }
104-132: Consider adding validation for synchronous invocation.The synchronous methods could benefit from additional validation to ensure they're only used in appropriate contexts.
Consider adding validation:
public static TResult FastInvoke<[DynamicallyAccessedMembers(JsonSerialized)] TResult>(this IJSRuntime jsRuntime, string identifier, params object?[]? args) { + // Validate that we're in a context where synchronous invocation is appropriate + if (!OperatingSystem.IsBrowser()) + { + throw new InvalidOperationException("Synchronous JavaScript invocation is only supported in the browser context."); + } + if (jsRuntime is IJSInProcessRuntime jsInProcessRuntime) return jsInProcessRuntime.Invoke<TResult>(identifier, args); throw new InvalidOperationException($"This operation is not available for the current instance of the JavaScript runtime: {jsRuntime.GetType()}"); }src/Butil/Bit.Butil/Publics/Navigator.cs (1)
93-133: Consider adding input validation for the Share and Vibrate methods.The action methods could benefit from input validation to prevent invalid JavaScript calls.
Consider adding validation:
public async Task<bool> Vibrate(int[] pattern) { + if (pattern == null || pattern.Length == 0) + { + throw new ArgumentException("Pattern array cannot be null or empty", nameof(pattern)); + } + if (pattern.Any(duration => duration < 0)) + { + throw new ArgumentException("Vibration durations cannot be negative", nameof(pattern)); + } return await js.FastInvokeAsync<bool>("BitButil.navigator.vibrate", pattern); } public async Task Share(ShareData data) { + if (data == null) + { + throw new ArgumentNullException(nameof(data)); + } + if (string.IsNullOrEmpty(data.Title) && string.IsNullOrEmpty(data.Text) && string.IsNullOrEmpty(data.Url)) + { + throw new ArgumentException("At least one of Title, Text, or URL must be provided", nameof(data)); + } await js.FastInvokeVoidAsync("BitButil.navigator.share", data); }src/Butil/Bit.Butil/Publics/History.cs (1)
Line range hint
121-183: Consider adding memory leak prevention for event handlers.While the event handlers are properly managed, there's room for improvement in preventing memory leaks.
Consider adding a weak reference collection:
-private readonly ConcurrentDictionary<Guid, Action<object>> _handlers = new(); +private readonly ConcurrentDictionary<Guid, WeakReference<Action<object>>> _handlers = new(); public async ValueTask<Guid> AddPopState(Action<object> handler) { var listenerId = HistoryListenersManager.AddListener(handler); - _handlers.TryAdd(listenerId, handler); + _handlers.TryAdd(listenerId, new WeakReference<Action<object>>(handler)); await js.FastInvokeVoidAsync("BitButil.history.addPopState", HistoryListenersManager.InvokeMethodName, listenerId); return listenerId; }src/Butil/Bit.Butil/Publics/VisualViewport.cs (1)
Line range hint
102-208: Consider optimizing event handler removal.The event handler removal could be optimized to reduce the number of JavaScript calls.
Consider batching the removals:
private async ValueTask RemoveAllEventHandlers() { if (_handlers.Count == 0) return; var ids = _handlers.Select(h => h.Key).ToArray(); _handlers.Clear(); VisualViewportListenersManager.RemoveListeners(ids); - var toAwait = new List<Task>(); - - var resizeValueTask = RemoveResizeFromJs(ids); - var scrollValueTask = RemoveScrollFromJs(ids); - - if (resizeValueTask.IsCompleted is false) - { - toAwait.Add(resizeValueTask.AsTask()); - } - - if (scrollValueTask.IsCompleted is false) - { - toAwait.Add(scrollValueTask.AsTask()); - } - - await Task.WhenAll(toAwait); + // Batch remove all event handlers in a single JS call + if (OperatingSystem.IsBrowser()) + { + await js.FastInvokeVoidAsync("BitButil.visualViewport.removeAllEventHandlers", ids); + } }This requires implementing a corresponding JavaScript method that handles both resize and scroll event removals in a single call.
src/Butil/Bit.Butil/Publics/Console.cs (1)
Line range hint
1-199: Consider adding error handling for JavaScript interop calls.While the implementation is correct, consider adding try-catch blocks to handle potential JavaScript interop failures, especially for methods that may fail in certain contexts (e.g., when the console API is not available).
Example implementation:
public async Task Assert(bool? condition, params object?[]? args) - => await js.FastInvokeVoidAsync("BitButil.console.assert", [condition, .. args]); + try + { + await js.FastInvokeVoidAsync("BitButil.console.assert", [condition, .. args]); + } + catch (JSException ex) + { + // Handle or rethrow with more context + throw new JSException("Failed to execute console.assert", ex); + }src/Butil/Bit.Butil/Publics/Window.cs (1)
267-267: Consider consolidating scroll method implementations.The scroll-related methods have duplicated logic for handling options vs. coordinates. Consider refactoring to reduce duplication.
Example implementation:
+private async Task ScrollInternal(string method, ScrollOptions? options, float? x, float? y) + => await js.FastInvokeVoidAsync($"BitButil.window.{method}", + options?.ToJsObject(), x, y); -public async Task Scroll(ScrollOptions? options) - => await js.FastInvokeVoidAsync("BitButil.window.scroll", - options?.ToJsObject(), null, null); +public async Task Scroll(ScrollOptions? options) + => await ScrollInternal("scroll", options, null, null); -public async Task Scroll(float? x, float? y) - => await js.FastInvokeVoidAsync("BitButil.window.scroll", null, x, y); +public async Task Scroll(float? x, float? y) + => await ScrollInternal("scroll", null, x, y); -public async Task ScrollBy(ScrollOptions? options) - => await js.FastInvokeVoidAsync("BitButil.window.scrollBy", - options?.ToJsObject(), null, null); +public async Task ScrollBy(ScrollOptions? options) + => await ScrollInternal("scrollBy", options, null, null); -public async Task ScrollBy(float? x, float? y) - => await js.FastInvokeVoidAsync("BitButil.window.scrollBy", null, x, y); +public async Task ScrollBy(float? x, float? y) + => await ScrollInternal("scrollBy", null, x, y);Also applies to: 274-274, 282-282, 289-289
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
src/Butil/Bit.Butil/Bit.Butil.csproj(1 hunks)src/Butil/Bit.Butil/Extensions/JSRuntimeExtensions.cs(1 hunks)src/Butil/Bit.Butil/Extensions/LinkerFlags.cs(1 hunks)src/Butil/Bit.Butil/Internals/JsInterops/EventsJsInterop.cs(2 hunks)src/Butil/Bit.Butil/Publics/Console.cs(7 hunks)src/Butil/Bit.Butil/Publics/Cookie.cs(2 hunks)src/Butil/Bit.Butil/Publics/Document.cs(7 hunks)src/Butil/Bit.Butil/Publics/ElementReferenceExtensions.cs(19 hunks)src/Butil/Bit.Butil/Publics/History.cs(10 hunks)src/Butil/Bit.Butil/Publics/Keyboard.cs(2 hunks)src/Butil/Bit.Butil/Publics/Location.cs(1 hunks)src/Butil/Bit.Butil/Publics/Navigator.cs(2 hunks)src/Butil/Bit.Butil/Publics/Notification.cs(3 hunks)src/Butil/Bit.Butil/Publics/Screen.cs(3 hunks)src/Butil/Bit.Butil/Publics/ScreenOrientation.cs(6 hunks)src/Butil/Bit.Butil/Publics/Storage/ButilStorage.cs(1 hunks)src/Butil/Bit.Butil/Publics/UserAgent.cs(1 hunks)src/Butil/Bit.Butil/Publics/VisualViewport.cs(11 hunks)src/Butil/Bit.Butil/Publics/Window.cs(4 hunks)
✅ Files skipped from review due to trivial changes (3)
- src/Butil/Bit.Butil/Internals/JsInterops/EventsJsInterop.cs
- src/Butil/Bit.Butil/Publics/Document.cs
- src/Butil/Bit.Butil/Publics/Location.cs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build and test
🔇 Additional comments (19)
src/Butil/Bit.Butil/Publics/UserAgent.cs (1)
19-19: LGTM! The change aligns with the PR objective.The switch to
FastInvokeAsyncis consistent with the optimization effort across the codebase.src/Butil/Bit.Butil/Extensions/LinkerFlags.cs (3)
7-10: LGTM! Well-defined flags for JSON serialization.The JsonSerialized constant correctly combines PublicConstructors, PublicFields, and PublicProperties flags needed for JSON serialization.
17-20: LGTM! Appropriate flags for JSInvokable types.The JSInvokable constant correctly targets public methods, which is appropriate for JavaScript-invokable members.
12-15: Consider being more specific with Component flags.Using DynamicallyAccessedMemberTypes.All might be overly permissive. Consider restricting to only the member types actually needed by components.
Run this script to analyze component usage:
src/Butil/Bit.Butil/Publics/Cookie.cs (2)
20-20: LGTM! Consistent with the optimization effort.The switch to FastInvokeAsync aligns with the PR objective.
65-65: LGTM! Appropriate use of FastInvokeVoidAsync.The switch to FastInvokeVoidAsync is correct for this void operation.
src/Butil/Bit.Butil/Publics/Keyboard.cs (2)
Line range hint
20-30: LGTM! Appropriate use of FastInvokeVoidAsync for event registration.The switch to FastInvokeVoidAsync is correct for this keyboard event registration.
80-80: LGTM! Proper cleanup with FastInvokeVoidAsync.The switch to FastInvokeVoidAsync is appropriate for cleanup operations.
src/Butil/Bit.Butil/Publics/Notification.cs (1)
19-19: Verify error handling behavior with FastInvoke methods.The switch to
FastInvokeAsync/FastInvokeVoidAsyncmaintains the method signatures while potentially improving performance. However, we should verify that error handling behavior remains consistent, especially for browser notification permissions and API availability.Also applies to: 29-29, 73-73
✅ Verification successful
Error handling is properly maintained with FastInvoke methods
The notification implementation has comprehensive error handling at multiple levels:
- Browser API compatibility check via
isSupported- Permission state management through enum mapping
- Notification display with service worker fallback
- JSON serialization error handling in FastInvokeAsync
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any existing error handling patterns with FastInvoke methods rg -A 5 "FastInvoke.*try|catch|throw"Length of output: 87678
Script:
#!/bin/bash # Search for notification-related FastInvoke implementations and error handling rg -A 5 "BitButil\.notification|FastInvoke.*notification"Length of output: 1773
Script:
#!/bin/bash # Search for BitButil.notification JavaScript implementation rg -g "*.js" -A 10 "BitButil\.notification\s*=|export\s+class\s+Notification"Length of output: 79
Script:
#!/bin/bash # Search for notification-related JavaScript/TypeScript files with broader patterns fd -e js -e ts | xargs rg -l "notification" # If files found, show their content echo "=== Notification implementations ===" rg -g "*.{js,ts}" -B 2 -A 5 "notification|Notification"Length of output: 7812
src/Butil/Bit.Butil/Publics/Screen.cs (1)
27-27: LGTM! Well-structured changes with proper thread safety.The changes maintain thread safety with
ConcurrentDictionarywhile potentially improving performance throughFastInvoke*methods. The event handling and cleanup logic is properly implemented.Also applies to: 35-35, 43-43, 51-51, 59-59, 67-67, 75-75, 89-89, 151-151
src/Butil/Bit.Butil/Extensions/JSRuntimeExtensions.cs (2)
1-9: LGTM!The imports are appropriate for the functionality being implemented, including necessary namespaces for JSON serialization, threading, and JS interop.
12-26: LGTM!The extension method is well-documented with XML comments and properly annotated with
RequiresUnreferencedCodeattribute for JSON serialization. The implementation correctly delegates to the overload with cancellation token.src/Butil/Bit.Butil/Publics/Navigator.cs (1)
Line range hint
20-89: LGTM!The property getter methods are well-documented and correctly use
FastInvokeAsyncwith appropriate return types that match the JavaScript API.src/Butil/Bit.Butil/Publics/History.cs (1)
Line range hint
26-107: LGTM!The History API methods are well-documented and correctly use the fast invocation methods. The implementation properly handles state management and URL validation through JavaScript.
src/Butil/Bit.Butil/Publics/VisualViewport.cs (1)
Line range hint
31-89: LGTM!The VisualViewport property getters are well-documented and correctly use
FastInvokeAsyncwith appropriate return types that match the JavaScript API.src/Butil/Bit.Butil/Publics/Console.cs (1)
14-14: LGTM! The console method implementations look correct.The changes consistently replace
InvokeVoidAsyncwithFastInvokeVoidAsyncacross all console methods while maintaining proper parameter passing and return types. The implementation properly handles:
- Null parameters using optional parameters
- Array parameters using params
- Method overloads
Also applies to: 22-22, 30-31, 39-40, 48-48, 57-57, 66-66, 74-74, 83-83, 93-93, 101-101, 109-109, 117-117, 126-127, 136-137, 145-146, 154-155, 163-164, 172-173, 181-182, 190-190, 198-198
src/Butil/Bit.Butil/Publics/Window.cs (1)
30-30: LGTM! The window method implementations look correct.The changes consistently replace
InvokeVoidAsync/InvokeAsyncwithFastInvokeVoidAsync/FastInvokeAsyncwhile maintaining proper:
- Parameter passing
- Return type handling
- Null parameter handling
- Complex object conversions
Also applies to: 39-39, 47-47, 55-55, 63-63, 72-72, 80-80, 87-87, 95-95, 103-103, 111-111, 119-119, 127-127, 135-135, 143-143, 151-151, 159-159, 167-167, 175-175, 183-183, 191-191, 204-204, 212-212, 220-220, 228-228, 236-236, 243-243, 251-251, 259-259, 267-267, 274-274, 282-282, 289-289, 297-297
src/Butil/Bit.Butil/Publics/ElementReferenceExtensions.cs (1)
40-40: LGTM! The element reference extension methods look correct.The changes consistently replace
InvokeVoidAsync/InvokeAsyncwithFastInvokeVoidAsync/FastInvokeAsyncwhile maintaining proper:
- Element reference handling
- Parameter passing
- Return type handling
- Enum conversions
Also applies to: 48-48, 56-56, 64-64, 72-72, 80-80, 88-88, 96-96, 104-104, 112-112, 120-120, 129-129, 137-137, 146-146, 153-153, 161-161, 168-168, 176-176, 183-183, 191-191, 199-199, 207-207, 215-215, 223-223, 230-230, 238-238, 245-245, 253-253, 261-261, 269-269, 277-277, 285-285, 292-292, 300-300, 307-307, 315-315, 323-323, 331-331, 339-339, 347-347, 355-355, 363-363, 372-372, 395-395, 404-404, 413-413, 436-436, 447-447, 479-479, 489-489, 511-511, 522-522, 531-531, 539-539, 546-546, 555-555, 588-588, 598-598, 607-607, 616-616, 624-624, 632-632, 639-639
src/Butil/Bit.Butil/Bit.Butil.csproj (1)
12-12: Verify the impact of suppressing IL2026 warning.IL2026 warns about requirements for trim-compatible serialization. Suppressing this warning might hide potential issues with JavaScript interop serialization in trimmed applications.
Run this script to check for trim-related attributes in the codebase:
✅ Verification successful
IL2026 warning suppression is properly handled ✅
The warning suppression is accompanied by appropriate trim compatibility attributes in the codebase. All JavaScript interop methods in
JSRuntimeExtensions.csare properly annotated with[RequiresUnreferencedCode]and[DynamicallyAccessedMembers]attributes, ensuring developers are aware of trim compatibility requirements despite the warning being suppressed.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for trim-related attributes that might need to be added rg --type-add 'cs:*.cs' --type cs -l 'DynamicallyAccessedMembers|RequiresUnreferencedCode|UnconditionalSuppressMessage.*IL2026' # Check for types used in JS interop that might need preservation ast-grep --pattern 'FastInvokeAsync<$type>($$$)'Length of output: 1022
Script:
#!/bin/bash # Check the content of JSRuntimeExtensions.cs in Bit.Butil to see how trimming is handled rg --type cs -A 5 -B 5 "RequiresUnreferencedCode|DynamicallyAccessedMembers" "src/Butil/Bit.Butil/Extensions/JSRuntimeExtensions.cs" # Also check if there are any [JsonSerializable] attributes that might need preservation rg --type cs "JsonSerializable" "src/Butil/Bit.Butil/"Length of output: 5305
1 participant
closes #9729
Summary by CodeRabbit
Based on the comprehensive summary, here are the release notes:
New Features
JSRuntimeExtensionsto enhance JavaScript function callsLinkerFlagsfor improved serialization and component handlingPerformance Improvements
Code Quality
These changes primarily focus on improving the underlying JavaScript interoperability infrastructure in the Bit.Butil library.