-
-
Notifications
You must be signed in to change notification settings - Fork 126
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
fix: application crash by clipping the recursion using WeakSet #1101
base: master
Are you sure you want to change the base?
fix: application crash by clipping the recursion using WeakSet #1101
Conversation
@AceTheCreator can I pick this for Level 1 Review ? |
…i-react into document-recursion-error-fix
Please go ahead @reachaadrika |
@catosaurusrex2003 Thanks for picking this issue . Also here I would suggest to go ahead with Also to fix the multiple recursions , rather than specifying the MAX DEPTH to 10 , where we might encounter issue you mentioned above , we can maybe explore FYR : https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/WeakSet PS : Open to discussion / suggestions on the approach , from both you and @AceTheCreator . cc : @AceTheCreator |
This is the perfect solution for recursion, thanks @reachaadrika Now since using WeakSet solves this specific issue, we wont require an Error Boundary for this. But having an Error boundary in our application will be beneficial in future. Should I include the Error boundary in this |
@catosaurusrex2003 Will review this change , once . Add yes adding Error Boundary , Would be beneficial in the long run . Let's keep this isolated you can create a new issue for the same as Improvement . I tested this for the same , @catosaurusrex2003 Let's also find more msgs like this and test 1-2 more samples |
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.
Let's test with 1-2 samples , Rest LGTM ✌🏻
@AceTheCreator Level -1 Review and Testing is done , Please review once too .
Thanks
Perfect 👍
while testing with the following yaml asyncapi: 3.0.0
info:
title: Account Service
version: 1.0.0
description: This service is in charge of processing user signups
channels:
userSignedup:
address: user/signedup
messages:
UserSignedUp:
$ref: '#/components/messages/UserSignedUp'
x-recursion: #this RECURSION clips correctly
$ref: '#'
operations:
sendUserSignedup:
action: send
channel:
$ref: '#/channels/userSignedup'
messages:
- $ref: '#/channels/userSignedup/messages/UserSignedUp'
x-recursion: # this RECURSION clips correctly
$ref: '#'
components:
messages:
UserSignedUp:
x-recursion: #this RECURSION clips correctly
$ref: '#'
payload:
x-recursion: # HERE <------------------------
$ref: '#'
type: object
properties:
x-recursion: #this RECURSION clips correctly
$ref: '#'
displayName:
type: string
description: Name of the user
email:
type: string I found all the recursions being handled correctly. except for the corner case recursion at this recursion is making the browser tab hang and go on in an infinite loop. weird . will look into its root cause and update the PR with a fix accordingly |
we can check this case once , Let me know if you require any help here |
The issue arises due to a cyclic dependency between the Schema and Extensions components, as observed in the following code: asyncapi-react/library/src/components/Schema.tsx Lines 361 to 363 in 076f36b
and asyncapi-react/library/src/components/Extensions.tsx Lines 29 to 31 in 076f36b
But this cyclic dependency is required for the components to render correctly. However, it's a rare edge case and might not need a fix. Instead we can choose to leave it as-is for now to avoid overcomplicating the solution. Cases like these reinforce the need of implementing an Error Boundary. This would allow us to fallback to a previous, correct version of the AsyncAPI document (similar to how undo works in the editor with Ctrl + Z). |
Agreed , Let's implement ErrorBoundary here . |
…i-react into document-recursion-error-fix
…osaurusrex2003/asyncapi-react into document-recursion-error-fix
I think this should be merged after #1108 is merged |
…i-react into document-recursion-error-fix
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!
Quality Gate passedIssues Measures |
I think we can move this PR forward now @reachaadrika ? |
Description:
This fixes the issue of website crashing due to too many recursions.
BUT
Even if we put a recursion limit of 10 to the function like this -> https://github.com/catosaurusrex2003/asyncapi-react/blob/9b73e41b7a644992851068d6cf327a5775eb7884/library/src/helpers/schema.ts#L521-L569
We will still have the recursive part rendering 10 times like this
The best approach would be to create an
ErrorBoundary
( using this ) on the whole application inlibrary/src/containers/AsyncApi/Layout.tsx
.So we can throw error indiscriminately instead of a empty object in
jsonFieldToSchema
function.We can render the error and make the app recoverable.
Let me know if my current approach is fine or should i modify my PR with the approach with an
ErrorBoundary
.Related issue(s)
#1100