Shifting complexity in expressions/hierarchy #943
Conversation
…ly resolved layout sets seems to be the ones in use (and using multiple of these functions will just serve to make refactoring harder)
… node/component/hierarchy is resolved or not, when the new default is that everything is resolved all the time. It's just at some points in the hierarchy.ts code that expressions are not yet resolved.
Using an enum (currently BaseValue) to indicate places in the layout that are supposed to have expressions instead of the fancy ExpressionOr<...> type that allowed for concrete values AND expressions. Now, the ILayoutComponent type itself does not support concrete values, expressions, or indicates that expressions are resolved. To do that, you have to wrap the type in ExprResolved<...> or ExprUnresolved<...> (the latter is deprecated, and we're hereby shifting the complexity of using non-resolved component definitions over to the older deprecated code). This makes working with the newer hierarchy system cleaner and easier than working with legacy code, hopefully increasing adoption and making for better developer experience.
Turns out the type I declared does devolves the enum to just a string, losing type-safety for the returnType assignment. I found and fixed the cases manually here. I tried, but failed, to enforce this using TypeScript, sadly.
This will better indicate the intended use for this enum, especially in component definitions.
olemartinorg
added
area/logic
…t resource bindings are automatically resolved according to row/index, so no need to run this again.
… and likert.ts cypress tests, so I'm not bothering fixing the mocking/test bootstrapping in order to give this real-world input so that it works again (the text variables are replaced by the node hierarchy now).
| layoutComponent?.getComponentType() === ComponentType.Presentation && | ||
| formComponentLegacy.type !== 'Summary' && | ||
| formComponentLegacy.type !== 'Group' | ||
| ) { |
There was a problem hiding this comment.
I don't think checking that the type is not Summary or Group is necessary here as they do not have a ComponentType, so they would fail because null !== ComponentType.Presentation.
There was a problem hiding this comment.
You're probably right! I did that to appease TypeScript, although it's obvious that the extra checks would always pass. I'll look over it once more, hopefully after you merge #940 and I do one more pass over this thing. Thanks for letting me know!
There was a problem hiding this comment.
Checked now, and it's still needed. TypeScript doesn't know that layoutComponent and formComponentLegacy is supposed to be the same thing (in many ways), so checking the type of layoutComponent does not narrow the type of formComponentLegacy (which is used in the implementation below). So these checks will make sure to narrow the type of formComponentLegacy so that GenericComponent can render it.
HanneLauritsen1967
assigned HanneLauritsen1967 and olemartinorg and unassigned HanneLauritsen1967
# Conflicts: # src/components/message/ErrorReport.tsx # src/features/pdf/AutomaticPDFLayout.tsx # src/features/pdf/CustomPDFLayout.tsx # src/utils/formComponentUtils.ts
|
SonarCloud Quality Gate failed.
|
Description
This PR is rather large, and contains somewhat complex type-changes. I apologize for the lack of readability! All in all, the main points of interest in this PR:
NodeTypetype that was used everywhere inhierarchy.types.d.ts. The point of this type was to make it possible to switch between resolved/unresolved properties (basically supporting having the full hierarchy tree represented with dynamic expressions intact, or with them evaluated). After Expressions optimization and streamlining #861, there's almost no usage of unresolved layout hierarchies, as accessing the full resolved tree got to be cheap and easy.hierarchy.ts, I'm moving some of them to a_privateexport, meant to symbolize that they're not really meant to be used outside of where they're in use now (unit tests). Please use the end result (the fully resolved, deep tree/hierarchy of components), not any of the intermediate steps.ExprResolvedtype we used to have was cool and all, but it performed somewhat poorly. When combining it with other types, especiallyMaybeSpecificIteminExprContext(again, coming from Expressions optimization and streamlining #861), you quickly reached the limits of TypeScript (or some other limit, who knows), and adding another layer depth of complexity to these types would break them. I don't like that the new, shining, and more future-proof framework we're building on is hard to use and extend, and after bumping my head against this thing, I decided to simplify the types for the layout hierarchy as much as I could, to hopefully invite further developments in that direction. To do that, I had to move/shift the complexity to the old-school methods of manipulating simple components/layouts directly. This is most clearly visible in this PR by the next bullet point:ILayoutComp*,ILayoutComponent) used to be defined such that they could contain either a concrete value (i.e.required: true) or an expression (i.e.required: ["equals", true, false]). This has now changed, and layout components are defined to have places that can contain expressions, but in order to actually use such an object, you at some point have to indicate to the type-system weather or not you expect these expressions to be resolved or not. Before, resolved expressions could be assignable directly to anILayoutComp*type, and you could sometimes get away with just using the layout redux state directly (in case you didn't have any expressions in the layout). This could cause bugs and requires code that is expression-aware to be the most complex. I've moved this complexity so that almost every code using the layout types need to wrap them in eitherExprResolved<...>(usually viaHComponentorAnyItem/LayoutNode) orExprUnresolved<...>. for resolved and unresolved (as-from-the-api) expressions, respectively.NodeTypegone, and with the complexity moved to legacy-style code via a forcedExprUnresolved<...>type, I found and deprecated a whole lot of code that I think should go away when we slowly clean up usages. It should also discourage further use of the older functionality, and encourage usage of the layout hierarchy.Related Issue(s)
Verification/QA
evalExpression()in the console, which had minor changes, and I tested and fixed the failing cypress tests)src/layout/layout.d.tsandlayout.schema.v1.json, and these are all backwards-compatiblekind/*label to this PR for proper release notes grouping