-
Notifications
You must be signed in to change notification settings - Fork 31
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
Faster save processing, reworking useNodeValidation()
#2724
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…here. Making sure to not render the presentation stuff inside itself.
…xt for each node. This should make the user experience around saving form data smoother.
…o longer marking state as not ready when processing save results, awaiting individual node validations in a separate function instead (waitForNodesToValidate). I had to make useWaitForState support async to make this work.
olemartinorg
added
the
kind/other
Pull requests containing chores/repo structure/other changes
label
Nov 19, 2024
…tect if the Presentation component has rendered above.
…s properly. Somehow the subform component started messing this up by
…uld have known, that client-side validation means that the data model validations will update also when that changes, not only when backend validations change. This makes comparing previous runs to skip selecting data too complex, so it would no longer give performance benefits.
Quality Gate passedIssues Measures |
bjosttveit
approved these changes
Nov 20, 2024
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.
Very nice!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
When form data is saved, we get new validations from the backend (when the app has implemented backend validation, at least). Previously, we saved the last processed validations pretty much everywhere (luckily only as a reference) so that we could make sure all
StoreValidationsInNodeWorker
components had done their job and stored new validations if needed. This caused lots of state updates on all nodes for every save operation, and worsened performance.Now, the
useNodeValidation()
hook will instead subscribe to the validation store and will only re-run validation when that changes. It will then save which backend validations have been processed into theRegistry
, so that we canuseWaitForNodesToValidate()
to wait until they have processed their validations (or, at least,useNodeValidation()
has run to completion).This also means that
NodesContext
no longer has to store the latest validations, and also means that it can now only be 'ready' or 'not ready' (no longer 'processing the last save'). The node generator will now only become 'not ready' after a save if new components are introduced (typically if a row has been added).In addition to this, a few minor changes. Either because they were tiny, or stuff I had to do because tests failed:
renderPresentation
flag in<Loader />
, and figuring out if the Presentation component has rendered via a Context. I've seen glimpses of double-PresentationComponent rendering lately, and this should fix it.<FormFirstPage />
as<Form />
will also detect that case and redirect you to the first page.navigate()
call in the Subform component that broke on me. It somehow started to remove the main form page from the URL when navigating to a relative path (.../Task_1/mainPage
navigating to the relativesubFormComponentId/subFormDataElementId
turned into.../Task_1/subFormComponentId/subFormDataElementId
).Related Issue(s)
Verification/QA
kind/*
label to this PR for proper release notes grouping…here. Making sure to not render the presentation stuff inside itself.