fix(engine): close #849 by implementing move hook #852
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
Benchmark resultsBase commit: lwc-engine-benchmark
|
This was referenced Nov 21, 2018
| @@ -117,8 +117,7 @@ export class SyntheticShadowRoot extends DocumentFragment implements ShadowRoot | |||
| return getHost(this).baseURI; | |||
| } | |||
| get isConnected(this: SyntheticShadowRootInterface) { | |||
| // @ts-ignore remove this after upgrading ts 3.x | |||
| return getHost(this).isConnected; | |||
| return (compareDocumentPosition.call(document, getHost(this)) & DOCUMENT_POSITION_CONTAINED_BY) !== 0; | |||
There was a problem hiding this comment.
What is this changing? Is this relevant to the PR?
There was a problem hiding this comment.
yeah, probably something that should have done in another PR, but there are failures on the accessibility lib that we have implemented for lightning namespace that is failing because this.template.isConnected is always undefined or false, that's because it is not implemented everywhere and we are not polyfilling it. This is the proper implementation that works across the board for shadows
ekashida
reviewed
Nov 21, 2018
| expect(elm.shadowRoot.textContent).toBe('5431'); | ||
| elm.sort('ASC'); | ||
| return Promise.resolve().then(() => { | ||
| expect(elm.shadowRoot.textContent).toBe('1345'); |
There was a problem hiding this comment.
What do we see before this change?
There was a problem hiding this comment.
it was throwing during the first rehydration because it was trying to repatch the element while moving it from one position to another.
caridy
commented
Nov 21, 2018
| @@ -34,7 +34,7 @@ export interface WireHash { | |||
| export interface RegisterDecoratorMeta { | |||
| readonly publicMethods?: string[]; | |||
| readonly publicProps?: PropsDef; | |||
| readonly track?: string[]; | |||
| readonly track?: TrackDef; | |||
There was a problem hiding this comment.
This was a mistake, diego was passing the right value from compiler, but we were typing that incorrectly.
|
@davidturissini you should re-review this once you are back (and the retargeting as well), but need to merge it to unblock LGC. |
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.
Details
We were using the
inserthook for the cases when we just move an existing node up or down in the children collection. This was causing the node to be re-patched, which can produces unpredictable results.Does this PR introduce a breaking change?