refactor/logic: Validate webpack upgrades are ready for React migration #1201
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
karrui
reviewed
Feb 22, 2021
liangyuanruo
force-pushed
the
refactor/logic
branch
2 times, most recently
from
408744d
to
5e1fb12
Compare
added 19 commits
March 9, 2021 11:36
liangyuanruo
force-pushed
the
refactor/logic
branch
from
fc6dfda
to
4c146f3
Compare
mantariksh
approved these changes
Mar 9, 2021
src/public/modules/forms/services/form-logic/form-logic.client.service.ts
Outdated
Show resolved
Hide resolved
src/types/form_logic.ts
Outdated
|
|
||
| type LogicCondition<K extends LogicField, V extends LogicConditionState> = [ | ||
| K, | ||
| Array<V>, |
There was a problem hiding this comment.
nit - we use [] notation rather than Array<> everywhere else in the repo
Suggested change
| Array<V>, | |
| V[], |
There was a problem hiding this comment.
Actually, I'd like to keep this to emphasize that LogicCondition is a two-element array. I'm also not sure if there's much value in enforcing T[] instead of Array<T> throughout the codebase, the former notation is a shorthand that emphasises iteration behavior, whereas the latter emphasises that arrays are actually higher-kinded types over another generic type. So I think both could be useful.
Merged
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.
Problems
PRs feat: Restore "Bulk Attachment Download Frontend (#640)" + webpack improvements #980 and fix/webpack: Allow ES8 syntax and automatically target IE11 #981 upgraded the frontend webpack build, which was supposed to allow the team to ship a mixture of AngularJS/JavaScript and TypeScript in the latest ES8 syntax without worrying about IE11 compatibility or polyfills.
There was also a lack of typing in client-side logic. This is dangerous when migrating to React, as logic conditions are wholly-determined on the frontend.
Solution
This PR validates those upgrades by demonstrating the conversion of the frontend FormLogic service to TypeScript. Controller code previously used to calculate which if-fields and then-conditions to display in the frontend have also been extracted into the service.
The type
LogicConditionshas also been introduced to constrain the correct pairing between field types and logical comparison operators as far as possible. It is itself composed ofCategoricalLogicCondition,BinaryLogicConditionandNumericalLogicConditiontypes to differentiate between the different logical operations. The types are not constructed from a statistical POV for simplicity (e.g. the Rating field is technically an ordinal data type) or for historical reasons (binary data is a special case of categorical data).Tests have also been added to ensure the above. Nevertheless, the changes so far only address frontend use cases; any server-side changes will be in a separate PR.
This PR also moved some JSDocs in the routes to be closer to the APIs that they are supposed to be describing.
Dependencies
A dev-dependency
jest-extendedwas introduced to take advantage of better utility functions for arrays not available injest.