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.
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
Epic/b2b commerce quotes #16345
Epic/b2b commerce quotes #16345
Changes from 250 commits
3737131
2d306bd
aee9ddb
7c006c5
e6d8eca
ebde45b
0a5a7f8
509a2dd
c094f64
a47bcd2
4f9d98c
d7bebfe
73cd440
12ed637
bad6226
5375acf
385150c
e7ba794
9ecf7e6
a187020
cbaac34
fa9c9a2
250417b
431dd24
1c2b5a9
79f5d92
117547d
be7be9a
705ba85
fa3e8bc
440c7b3
ed20f46
aba712f
27f7458
aac86eb
461fe22
a156860
8f5f597
aa3e366
b0a1583
ac37ca1
501cb96
289aa0f
a6b61d2
a24c5a6
60ebba1
c326749
89b9bad
fb843fe
7b70d06
df984db
6e73db3
223a8a5
084c5c0
0372a5c
8e3078b
c43e19e
1f5fe44
0b98e00
7f94533
40715d3
0c5e610
58ff7e3
19551e5
762ba96
7710732
70c98ea
632117b
0348c22
6e714c9
c2d0ae8
d8b0e2d
c6aaac1
b2c6921
ff369b2
547c09d
3d4d268
cc00b8e
5a7e609
7cd1e82
6e5b04c
bdaea25
04a681c
28c27ba
d8d726d
99cbc81
434ba5a
37b3bdb
9c70a3e
0cb44de
e94b611
af354b8
8a0ec22
0f8c33a
aa01e1a
25badab
10d7434
3319a04
1187ab8
84864f2
e419d74
669cadb
39339ac
28cb82d
e711eb0
7ffb051
a7815c8
202791b
77ef764
e5309c3
833f7de
981e856
ac20ba5
1247fa4
0d722dd
b572232
d99d083
6227844
b995115
8b748c5
68b0941
c2ac68d
bdd1244
5c78325
cb1a509
e2f31fc
ae802c9
36a5500
dc5bccf
b596921
bc5362f
bf47d01
a480a97
978cabc
c28dd84
4b0ec3a
85d3f30
c1fbc79
84d7756
fa1d021
1cf5671
f7687cc
8b17aa5
8fc8f69
906ef6c
971fc55
1a4e2e0
b661e42
83abd83
23a5ec8
e78de39
2fe4e2e
eb18843
96b821b
c7dbde2
74a31b4
c71d82f
c4feabc
e02081d
5ca1817
434a062
4cc828a
6d7a92b
238f8f9
d126666
ece0dd9
886ca0f
da0a342
c66521c
0a9ae36
a074806
c459982
1a5753a
2de020a
ef1d051
543ed08
ca5cf0c
689dd8b
d9f2ddf
396881e
1f14dfd
1bd30d9
fea2f22
3696ae9
9550d56
132a8f1
5dedd76
feb7bce
492af29
5947966
9189430
d56080a
05325ec
f4a6a5f
ede7ee4
6fdb63e
fb8bfd5
0e13f6d
263c1a7
ca77027
1ab3c05
cdb6588
b9d0171
eb57ac0
437034a
7bc46d9
ed787c1
e548811
702a83e
abe8382
bdc962e
ce18f51
7e0dfc5
f7f2154
e6ad2fe
72a05be
86d0b14
07f8bea
7643f36
3187622
33a7ce9
50ce095
637b190
b1d7cfa
7de83ce
4c49539
8dc3ffd
a9bc555
fbe11ef
e035c9e
8a80d1f
0de417e
d300832
1ec729b
b9acefa
937c386
bf7387e
8c988d2
eb4bc21
46c91b4
a2c61cf
b8549fc
d6e8a8c
6fb9fb6
f49619c
766f286
499c334
0c92219
1f9375c
649e639
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
[MEDIUM]
Introducing this
_forceRerender
property which couples methodsgetInputsFromContext()
withrerenderChangedItems()
makes it much harder for me to understand how the component works.I guess, it was introduced to workaround to some issue with change detection.
What other alternatives did we consider to fix the issue. What pros and cons of them do you see?
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.
@Platonn As far as i remember the issue was the following. As you can see in the type script doc of the
rerenderChangedItem
method, the component only re-renders any cart item when it was actually changed in an attempt to optimize performanceFor quote there is one case where this is not sufficient. In case a quote is switched from read-only to edit mode or vice versa, a re-render is also required, as an item is rendered differently in read-only mode compared to edit mode. For example in edit mode there is a stepper to enter a quantity, while the same control is disabled in read-only mode.
So when switching the quote from read-only to edit mode, the state of the read-only flag in the context will change and the component shall re-render.
The way its implemented is in my eyes the straight forward way of fixing the issue. In case the read-only flag from context switches, we need to re-render any item once, no matter whether the item was changed or not.
But maybe i'm missing here something, so let me know if you have any other idea, better way of fixing it.
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.
I agree your solution is probably the least invasive one that could exist. It's because the existing TS of this component is already complex. Unfortunately, it the proposed solution makes the component also a bit harder to reason about, and is not correct in 100% cases (e.g. when
readonly
property is changed via a regular@Input()
but not via an injectable Context).Here's how I understand the reasons of the existing complexity:
@Input()
s passed by a parent componentThe values from the injectable Context are copied to the regular properties being
@Input()
s@Input()
properties triggers a complex logic, e.g. the setter of the propteryitems
triggers 2 functions:resolveItems(items)
andcreateForm()
(which depend only on the value ofitems
).isCartLodaing
depends on the value ofreadonly
(see source) - please note that in this case the order updating values of propertiesisCartLoading
andreadonly
matter. In particular, if the logic for the setter ofisCartLoading
may use the outdated value ofreadonly
, if the propertyreadonly
is updated only after theisCartLoading
.This PR adds up to the complexity, by introducing the property
_forceRerender
which is set on change of the propertyreadonly
, but used (and reset) in the setter's logic of the propertyitems
:In this PR, we propose that the function
resolveItems(items)
triggered by the setter of the propertyitems
now depends on the value ofreadonly
(or to be precise: on the fact whether the propertyreadonly
just have changed). This is prone to a race condition issues, if the propertyreadonly
is updated only after the propertyitems
is updated - because then the logic ofresolveItems(items)
won't know yet about the latest value (and also about the change) of the propertyreadonly
.Moreover this PR proposes to save an information about a change of the property
readonly
to the boolean property_forceRerender
only, when the value ofreadonly
is changed only by the Context. But to be 100% correct, we should behave similarly, also if the propertyreadonly
is changed via a regular@Input()
- this is not handled now, as far as I understand from the code.The information about the change of the property
readonly
is stored in the class state, in a property_forceRerender
. Then this property is reset to false by some other method. Ideally the such a volatile information as "property has changed" should be passed directly as an argument to a logic that needs this information at the moment of the change. Storing it in a property and then resetting in some other method makes the class logic harder to reason about.Now we call
cdr.markForCheck()
only after the change of the propertyreadonly
, but not in the case of chaning other properties, which is not obvious why, on the first sight. Btw. [QUESTION]: Do we really need to callmarkForCheck()
when propertyreadonly
changes? If yes, don't we need to do it for the update of any other property?I can understand that because of the existing complexity of this class, it's challenging to find a simple solution to a bug (bug is the following, as far as I understood: the form controls are not updated in the html view when the property
readonly
changes its value).To avoid having race conditions in terms of setting various input properties, we should avoid triggering any setters logic on individual input properties, but instead trigger all such logic only after all inputs are updated. In particular, we should do it in those moments:
getInputsFromContext()
@Input()
s passed by a parent component - inngOnChanges()
lifecycle hookHere's a snippet of code illustrating this idea:
All that said, I like the solution proposed originally in this PR for its brevity. Unfortunately it has some considerable cons and is prone to mistakes. I'm not forcing my above mentioned snippet of code and I didn't test it. I wanted to visualize my thoughts. But the most I'd like to hear your feedback. Then we can come together to some agreement that should serve the long-term health of this class.
I'm looking forward to hearing your honest feedback. Thanks in advance.
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.
I have not seen this component before in detail, therefore hopefully I am not missing anything:
I understand your concerns Kris, agree that the component is too complex already now (and maybe depends on some assumptions that are not always met), and that the quote extension even increases the concerns. What you propose sounds plausible.
However I see 2 risks for the refactoring: (1) Applying the feature flags for covering the breaking changes will again increase the complexity and too is error-prone
(2) We are probably not aware of all possible cart features and usages of this components, so we could create regressions.
In case we do the the proposed refactoring, we also need to involve someone who has a bit of knowledge about all SPA cart features..(or would that be you Kris?). That would at least mitigate the second risk. What do you think @Uli-Tiger and @Platonn ?
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.
If i remove the markForCheck() the issue occurs again. I tested a bit and it seems that it needs to be called before the items setter is called, which in turn triggers the re-render logic. The other properties will not trigger a re-render, so i guess it is not required in that case.
PS: Please also consider that markForCheck() is already called in setLoading setter as side effect dependent on the readonly state of the cart. Since in our case isLoading is undefined, we need the explicit call.
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.
I refactored the code a bit to address at least some concerns, but still keeping the amount of changes limited, so that i have confidence that my changes do not break anything, especially considering that i do not know all the use case of the component. Especially the _forceReRender property was eliminated, instead passing on the flag as optional method parameter.
The fact that i do not know all the use cases of the component, and that i do not know why it was written in this way in the first place is for strong indicator to avoid a full refactoring potentially breaking scenarios i do not know or understand.