fix: context property observation, tests #5536
Conversation
| ], | ||
| }); | ||
| // Legacy SSRv1 does not support context when inherited | ||
| if (!process.env.ENGINE_SERVER) { |
There was a problem hiding this comment.
Inherited context doesn't work for legacy SSR (V1), but it never did. Testing for this results in a number of hydration errors for V1 only.
| fieldsOrProps: { | ||
| [name: string]: any; | ||
| } |
There was a problem hiding this comment.
How about defining a type that we can re-use for both connect/disconnect :
type VMFieldsOrProps = VM['cmpFields'] | VM['cmpProps'];
fieldsOrProps: VMFieldsOrProps
There was a problem hiding this comment.
Thanks, changed the function signature to to the gate. Original had to be so reverted to that.
| // 1. The template change results in the parent component being marked as dirty. | ||
| // 1a. Marking the parent as dirty sets the currentReactiveObserver to the parent, here: https://github.com/salesforce/lwc/blob/master/packages/%40lwc/engine-core/src/libs/mutation-tracker/index.ts#L83 | ||
| // 2. The new template doesn't contain the child so disconnectContext is called on the child component. The BUG: If the child properties are incorrectly observed then riggering disconnectContext marks all child properties | ||
| // for observation using the currentReactiveObserver of the parent set in 1a. here: https://github.com/salesforce/lwc/blob/master/packages/%40lwc/engine-core/src/libs/mutation-tracker/index.ts#L60 | ||
| // 3. Next, a delayed property mutation inside the child component's renderedCallback occurs and this delayed (post disconnection) mutation triggers valueMutated, where all the parent properties are registered listeners. | ||
| // That happens here: https://github.com/salesforce/lwc/blob/master/packages/%40lwc/engine-core/src/libs/mutation-tracker/index.ts#L41 | ||
| // 3b. This causes the parent component to re-render. |
There was a problem hiding this comment.
@jhefferman-sfdc can you check if enabling this flag will fix the issue as well?
There was a problem hiding this comment.
It does not. See tests here:
- with bug (same)
- without bug (same)
|
|
||
| const enumerableKeys = keys(getPrototypeOf(component)); | ||
| const enumerableKeys = keys(fieldsOrProps); | ||
| const contextfulKeys = ArrayFilter.call(enumerableKeys, (enumerableKey) => | ||
| isTrustedContext((component as any)[enumerableKey]) | ||
| isTrustedContext(fieldsOrProps[enumerableKey]) | ||
| ); |
There was a problem hiding this comment.
With this change we're only including the values in cmpProps and cmpFields while previously we were looking for all of the fields in the component instance prototype chain.
@jhefferman-sfdc just want to confirm, do we only care about the fields in cmpProps and cmpFields for context?
There was a problem hiding this comment.
With this change we're only including the values in
cmpPropsandcmpFieldswhile previously we were looking for all of the fields in the component instance prototype chain.@jhefferman-sfdc just want to confirm, do we only care about the fields in
cmpPropsandcmpFieldsfor context?
@jmsjtu are you referring to inherited context? We do care about this but the respective objects include it. See related test here. I checked manually too for decorated and non-decorated.
There was a problem hiding this comment.
Posting for posterity, we discussed offline.
@jmsjtu are you referring to inherited context? We do care about this but the respective objects include it. See related test #5536 (comment). I checked manually too for decorated and non-decorated.
I actually meant going from keys(getPrototypeOf(component)); 👉 keys(fieldsOrProps); we're now only looking at the field defined in the class instead of all the properties on the prototype.
We don't think it should be an issue though as context needs to be set on the class that extends LightningElement
Details
Fixes a property observation issue in the context connection and disconnection. By accessing the component properties, they are marked for observation and in very specific scenarios (see accompanying tests) this can become an issue and cause components to re-render when they shouldn't.
Does this pull request introduce a breaking change?
Does this pull request introduce an observable change?
GUS work item
W-19830319