Expressions optimization and streamlining #861
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
added 30 commits
January 16, 2023 13:17
…d to find the current layout for some reason - and the error resulted in a TypeError on the console.
… than just the default value returned by an expression (for that property). This is one of the steps needed to make resolveNodesInLayouts() work per-row for repeating groups, which is also a step in the right direction when "caching" expression results using a React context as a resolved nodes provider.
…). This adds a new groupExpressions property for each row object (where only the properties that contained expressions, and were resolved per-row are added).
…now just does a lookup in the new ExprContext (which contains the full hierarchy). This makes usage much faster, as we've calculated all expressions in advance. This model should also replace useExpressions().
…being resolved in a top-level context (using the ExprContext). Lots of stuff is bound to fail hard right now, as this refactor is just barely holding its own weight.
…lvedNodesInLayout, they did not have their textResourceBindings resolved and replaced - making text bindings using variables inside repeating groups not work properly. Fixing this by adding that functionality to the hierarchy. Removing the test and simplifying the hook in a way where it doesn't really need unit-testing.
…when running tests
…ctacularly when I rewrote GenericComponent, potentially because the 'layout' property disappeared for some reason (not sure if Likert is nesting GenericComponent or what is happening). Mixing in passed props (but overwriting with evaluated layout properties) seems to fix it.
…the state was not being set correctly in redux. Also marking rows as potentially undefined. They aren't when listing out each value, but you should always do null-checking when accessing a row by index.
…assed through the component properties alone, while not being present in the redux layout state.
… via redux layout as well
…behave the same as in createRepeatingGroupComponents()
…mmaryGroupComponent did not match the actual likert repeating group rows. This happened because the component id did not include the correct row index suffix, so the node lookup returned the first row instead of the specific row.
…to be found when looking for the legacy component
…when largeGroup is set
# Conflicts: # src/features/form/containers/GroupContainer.tsx # src/features/form/containers/RepeatingGroupsEditContainer.tsx
…n the 'children' property, as well as prevent it from sorting groups last. This new variant might also be a bit faster, is shorter, and hopefully easier to understand. This is one of the steps needed to fix the summary.ts cypress test, as rewriting to use nodes revealed the hierarchy also changed the order from the definition in the layout structure.
…y mapped to the real component ids
…d preferably be replaced by the LayoutNode/hierarchy tools
…ed as repeating, and SummaryGroupComponent never really cared about that until now.
olemartinorg
added
the
kind/product-feature
…o using node layouts instead of flat layouts, proper component IDs, exposing ExprContext everywhere (but making it undefined when it's not possible to construct it).
4 tasks
bjosttveit
reviewed
Jan 26, 2023
bjosttveit
approved these changes
Jan 26, 2023
|
@bjosttveit Yup, sorry, it's a huge change that affects everything... The fact that you spotted that multiPageIndex thing means you probably looked more closely at this than I would! 🥳 |
# Conflicts: # src/layout/Likert/RepeatingGroupsLikertContainer.tsx
|
Kudos, SonarCloud Quality Gate passed! |
This was referenced Feb 21, 2023
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
Marking this as a product feature, because it opens up a few more things:
titleandtableTitleintextResourceBindingsvia expressions now, which did not work correctly before (at least it crashed the app for me when I tested on earlier versions).titlewhen displaying a component in aSummary(which did not always seem to look up variables in text resources before, but does now).With that said, some bullet points detailing some of the more technical changes here:
useExpressions()anduseExpressionsForComponent(), most expressions are now evaluated once in theExprContext(which just loadsresolvedNodesInLayouts()and provides the result to all child components via two new hooks -useExprContext()anduseResolvedNode()). This is what gives us the much-needed performance boost.Summary, where the node hierarchy now replaces most component config wrangling.edit.deleteButton) were evaluated per-row (as this is a property on theGroupcomponent, but could have different results per-row). Now, instead of just a default value (if the expression fails to evaluate), you have to pass in a configuration for every possible expression. This configuration tells the expression engine if an expression should be evaluated per-row or not, so expressions likeedit.deleteButtonwill stay unresolved in theGroupcomponent and will be evaluated and available in thenode.item.row[123].groupExpressions.edit.deleteButtonproperty instead (for each row index). This also fixes an issue where these expressions were being resolved in unrelated code, and could end up failing (because, for example, acomponentlookup was not available when not in a row context), resulting in an error message in the developer console.Related Issue(s)
Verification
addedupdatedsrc/layout/layout.d.tsandlayout.schema.v1.json, and these are all backwards-compatible