Adding analyzer to warn about JSInterop calls not wrapped in a try catch block - #67900
Conversation
… to string type names. Add more tests accordingly.
… block with at least one catch as guarded. Adding tests accordingly.
…ncompatible code.
…c, InvokeConstructorAsync).
…dd some tests for calls inside lambda expressions.
…erent types of exceptions thrown.
…JSDisconnectedException or generic Exception.
Co-authored-by: Youssef Fahmy <youssefvictor00@gmail.com>
…eption, JSDisconnectedException or generic Exception." This reverts commit 48fa81a.
… be different types of exceptions thrown." This reverts commit 218e570.
Co-authored-by: Ilona Tomkowicz <32700855+ilonatommy@users.noreply.github.com>
….com/MayaKirova/aspnetcore into mkirova/unguarded-JSInterop-analyzer
Co-authored-by: Ilona Tomkowicz <32700855+ilonatommy@users.noreply.github.com>
….com/MayaKirova/aspnetcore into mkirova/unguarded-JSInterop-analyzer
|
Thanks for your PR, @MayaKirova. Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
There was a problem hiding this comment.
Pull request overview
This PR adds a new Components analyzer (and code fix) to warn when JavaScript interop calls aren’t wrapped in a try/catch, aiming to improve resiliency in scenarios like prerendering and disconnected clients.
Changes:
- Added
JSInteropAnalyzerto report a warning for unguarded JS interop invocations. - Added
JSInteropCodeFixProviderplus new unit tests covering diagnostics and fix behavior. - Updated template sample and diagnostics documentation to reflect the new guidance.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ProjectTemplates/Web.ProjectTemplates/content/RazorClassLibrary-CSharp/ExampleJsInterop.cs | Updates template sample to wrap JS interop calls in try/catch and provide clearer failures. |
| src/Components/Analyzers/test/JSInteropCodeFixProviderTest.cs | Adds tests validating the new code fix behavior. |
| src/Components/Analyzers/test/JSInteropAnalyzerTest.cs | Adds tests validating diagnostics for various JS interop call shapes and contexts. |
| src/Components/Analyzers/src/Resources.resx | Adds localized strings for the new diagnostic and code fix. |
| src/Components/Analyzers/src/JSInteropCodeFixProvider.cs | Introduces a code fix to wrap eligible JS interop calls in try/catch. |
| src/Components/Analyzers/src/JSInteropAnalyzer.cs | Introduces the analyzer logic for detecting unguarded JS interop calls. |
| src/Components/Analyzers/src/DiagnosticDescriptors.cs | Registers the new diagnostic descriptor and assigns an ID. |
| docs/list-of-diagnostics.md | Updates the published diagnostic list/range for BL diagnostics. |
| SyntaxFactory.CatchClause() | ||
| .WithCatchKeyword(SyntaxFactory.Token(indentList, SyntaxKind.CatchKeyword, SyntaxFactory.TriviaList(SyntaxFactory.Space))) | ||
| .WithDeclaration( | ||
| SyntaxFactory.CatchDeclaration( | ||
| SyntaxFactory.ParseTypeName("Exception")) | ||
| .WithCloseParenToken(SyntaxFactory.Token(SyntaxTriviaList.Empty, SyntaxKind.CloseParenToken, eolList))) | ||
| .WithBlock( | ||
| SyntaxFactory.Block() | ||
| .WithOpenBraceToken(SyntaxFactory.Token(indentList, SyntaxKind.OpenBraceToken, eolList)) | ||
| .WithCloseBraceToken(SyntaxFactory.Token(indentList, SyntaxKind.CloseBraceToken, SyntaxTriviaList.Empty)))), |
|
@MayaKirova, we have to wait for #67898, then we can merge/rerun. |
Head branch was pushed to by a user without write access
| catch (JSException ex) | ||
| { | ||
| throw new InvalidOperationException("Unable to invoke the web worker JavaScript function.", ex); | ||
| } |
There was a problem hiding this comment.
@ilonatommy I'm not sure how is this better than letting the call throw, if an exception is going to propagate up anyways?
There was a problem hiding this comment.
It's better because it prevents the warnings that are treated as errors in the template tests and would fail the CI. The alternative would be to supress this warning but that's not a good practice for the template code. User can decide to swallow the exception if they want but I don't think we should promote such a solution in the template.
Adding analyzer to warn about JSInterop calls not wrapped in a try catch block
Description
Added analyzer that shows warnning and code fixer that wraps the JSInterop calls (InvokeAsync, InvokeVoidAsync etc.) in try/catch block.
Also added some tests:
IJSRuntimeextension call (InvokeVoidAsync)IJSObjectReferenceextension call (InvokeVoidAsync)IJSRuntime.InvokeAsync<T>callIJSObjectReference.InvokeAsync<T>callIJSInProcessRuntime.Invoke<T>callIJSInProcessObjectReference.Invoke<T>calltry/catchifinsidetry/catch)InvokeVoidAsynctry/catchInvokeAsync<T>try/catchInvokeVoidAsynctry/catchFixes #67306