feat(engine): getRootNode in patched nodes #786
Merged
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
jodarove
force-pushed
the
jodarove/get-root-node-revisit
branch
from
2176e59
to
da13671
Compare
Benchmark resultsBase commit: lwc-engine-benchmark
|
|
comments from the previous pull request: we currently have one implementation of getRootNode, which is used in the events from faux-shadow: https://github.com/salesforce/lwc/blob/master/packages/lwc-engine/src/faux-shadow/node.ts#L65 which currently have some issues:
it was agreed that we should change the existing getRootNode to match the native behavior. although it might involve some extra work cause will change from returning the host to return the shadow |
jodarove
commented
Oct 30, 2018
| // it is coming from a slotted element | ||
| isChildNode(getRootNode.call(target, event), currentTarget as Node) || | ||
| // it is not composed and its is coming from from shadow | ||
| (composed === false && getRootNode.call(target) === currentTarget) |
There was a problem hiding this comment.
here the second call to getRootNode.call(target) can be cached, since const { composed } = event as any; and we are ensuring that composed === false that's why the if condition was divided into 2, so the getRootNode only had to be calculated only once
jodarove
commented
Oct 30, 2018
jodarove
commented
Oct 30, 2018
|
@caridy @davidturissini @ekashida some things missing, but the general idea can be reviewed if you have time. |
Benchmark resultsBase commit: lwc-engine-benchmark
|
|
@davidturissini should definitely look at this. Base on a very quick pass, it LGTM, but I have to dig in more. |
jodarove
force-pushed
the
jodarove/get-root-node-revisit
branch
from
d3a0baa
to
3d3c1b6
Compare
Benchmark resultsBase commit: lwc-engine-benchmark
|
Benchmark resultsBase commit: lwc-engine-benchmark
|
|
thanks @caridy , this is ready for a full review. fyi/ @davidturissini @ekashida |
caridy
reviewed
Oct 31, 2018
caridy
reviewed
Oct 31, 2018
implementation. Adds the getRootNode for patches nodes.
missing: - check if #750 is resolved, if that is the case, add a test for it. - check types in function calls. - refactor and organize anything i might have left.
should be called when the event comes from within nested slots. fixes: #750
of names, and use GetRootNodeOptions type.
jodarove
force-pushed
the
jodarove/get-root-node-revisit
branch
from
25c77f8
to
40b5ec9
Compare
jodarove
force-pushed
the
jodarove/get-root-node-revisit
branch
from
40b5ec9
to
f5175b1
Compare
Benchmark resultsBase commit: lwc-engine-benchmark
|
Benchmark resultsBase commit: lwc-engine-benchmark
|
caridy
reviewed
Nov 1, 2018
davidturissini
suggested changes
Nov 1, 2018
|
|
||
| // is SyntheticShadowRootInterface | ||
| if ('mode' in rootNode && 'delegatesFocus' in rootNode) { | ||
| rootNode = getHost(rootNode); |
There was a problem hiding this comment.
Why do we need this check? This function should never run in native mode. The only thing I can think of is if getRootNode returns a document. It seems strange for this method to either return an Element or document?
There was a problem hiding this comment.
when getRootNode with composed = true or getRootNode in an lwc root element ? in those cases it will return a document, this check is for those cases. this method returns a Node
| @@ -240,6 +240,10 @@ export class SyntheticShadowRoot extends DocumentFragment implements ShadowRoot | |||
| getSelection(this: SyntheticShadowRootInterface): Selection | null { | |||
| throw new Error(); | |||
| } | |||
|
|
|||
| getRootNode(options?: GetRootNodeOptions): Node { | |||
| return getRootNode.call(this, options); | |||
There was a problem hiding this comment.
add this type. Do we need the indirection here or can we just implement getRootNode right here?
There was a problem hiding this comment.
@davidturissini i added the this type (on both). what i don't fully understand is:
can we just implement getRootNode right here
getRootNode (now getRootNodeGetter) is used in some other places, and not alway on a patched element (for ex: events) would an implementation here will lead to duplicated code? can you explain more so i can understand better?
| @@ -323,6 +367,9 @@ export function PatchedNode(node: Node): NodeConstructor { | |||
| } | |||
| return parentNode; | |||
| } | |||
| getRootNode(options?: GetRootNodeOptions): Node { | |||
| return getRootNode.call(this, options); | |||
There was a problem hiding this comment.
this type. Same question as above. I wonder if we need the indirection or can we simple implement here
- remove abstractions to improve test readability. - add a test for when elements are added to the shadow with innerHTML. - renamed the regular method to getRootNodeGetter to have the this. - added the this type to Node.getRootNode and SyntheticShadowRoot.getRootNode
Benchmark resultsBase commit: lwc-engine-benchmark
|
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.
implementation.
Adds the getRootNode for patches nodes.
Details
Reopening: #744
Changes enclosed:
Does this PR introduce a breaking change?