Skip to content
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

fix(engine): never use a dot notation of patched objects #410

Merged
merged 1 commit into from
Jun 13, 2018

Conversation

caridy
Copy link
Contributor

@caridy caridy commented Jun 13, 2018

Details

  • we were applying the proper logic on the wrong values by accessing the properties of a patched object.
  • additionally we were not giving the proper enumerability and configurability for the patched descriptors.

Does this PR introduce a breaking change?

  • No

@@ -36,7 +36,9 @@ const eventShadowDescriptors: PropertyDescriptorMap = {
return currentTarget;
}
return patchShadowDomTraversalMethods(currentTarget);
}
},
enumerable: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ravijayaramappa this fixes the issue that you mentioned for target and current target.

@@ -182,7 +185,7 @@ function getWrappedRootListener(vm: VM, listener: EventListener): WrappedListene
// it is coming from an 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(event.target) === currentTarget)) {
(composed === false && getRootNode.call(target) === currentTarget)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we were using the event.target that was returning an already retargeted target.

@@ -318,7 +321,8 @@ function detachDOMListener(vm: VM, type: string, wrappedListener: WrappedListene

const NON_COMPOSED = { composed: false };
export function isValidEventForCustomElement(event: Event): boolean {
const { target, currentTarget } = event;
const target = eventTargetGetter.call(event);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

never use the . notation at the low level, use the cached getter to avoid getting a patched value.

@salesforce-best-lwc-internal
Copy link

Benchmark results

Base commit: 369e073 | Target commit: 85dc205

lwc-engine-benchmark

table-append-1k metric base(369e073) target(85dc205) trend
benchmark-table/append/1k duration 141.90 (± 5.00 ms) 141.20 (± 3.60 ms) 0.49% 👌
table-clear-1k metric base(369e073) target(85dc205) trend
benchmark-table/clear/1k duration 11.35 (± 0.50 ms) 11.30 (± 0.50 ms) 0.44% 👌
table-create-10k metric base(369e073) target(85dc205) trend
benchmark-table/create/10k duration 832.40 (± 4.10 ms) 833.60 (± 3.90 ms) -0.14% 👌
table-create-1k metric base(369e073) target(85dc205) trend
benchmark-table/create/1k duration 100.00 (± 2.10 ms) 100.35 (± 2.80 ms) -0.35% 👌
table-update-10th-1k metric base(369e073) target(85dc205) trend
benchmark-table/update-10th/1k duration 84.20 (± 4.45 ms) 85.90 (± 5.60 ms) -2.02% 👌
tablecmp-append-1k metric base(369e073) target(85dc205) trend
benchmark-table-component/append/1k duration 240.20 (± 5.00 ms) 243.40 (± 3.90 ms) -1.33% 👎
tablecmp-clear-1k metric base(369e073) target(85dc205) trend
benchmark-table/clear/1k duration 36.10 (± 1.50 ms) 35.10 (± 1.60 ms) 2.77% 👍
tablecmp-create-10k metric base(369e073) target(85dc205) trend
benchmark-table-component/create/10k duration 1700.80 (± 9.50 ms) 1695.40 (± 9.20 ms) 0.32% 👌
tablecmp-create-1k metric base(369e073) target(85dc205) trend
benchmark-table-component/create/1k duration 196.30 (± 4.10 ms) 195.20 (± 4.60 ms) 0.56% 👌
tablecmp-update-10th-1k metric base(369e073) target(85dc205) trend
benchmark-table-component/update-10th/1k duration 75.70 (± 3.50 ms) 75.70 (± 4.30 ms) 0.00% 👌

@caridy caridy merged commit 2c6b1f4 into master Jun 13, 2018
@caridy caridy deleted the caridy/event-bug-target-and-current-target branch June 13, 2018 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants