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

refactor(engine): implementation of wired reform #1417

Closed
wants to merge 1 commit into from

Conversation

caridy
Copy link
Contributor

@caridy caridy commented Jul 17, 2019

EXPERIMENTAL_REFACTOR

Details

First refactor to slowly iron out the way a wire adapter sets values into a wired fields. In this first step, we transform the behavior of the value set into a field from a reactive proxy to a readonly proxy.

Does this PR introduce breaking changes?

  • 🚨 Yes, it does introduce breaking changes.

If yes, please describe the impact and migration path for existing applications.

The potential impact here is for someone wiring to a field, and making manual mutations to the value that was wired. The workaround is to split out that logic into a tracked value, while keeping the wired field as set by the adapter.

The PR fulfills these requirements:

  • Have tests for the proposed changes been added? ✅
  • Have you followed these instructions to clearly describe the issue being fixed or feature enhanced? ✅

cmpSlots: SlotSet;
cmpTrack: any;
cmpFields: Record<string, any>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is just a rename since the cmpTrack was exclusively a hash table for tracked fields, but in general, since they do not overlap, we can just generalize that for all important fields.

@@ -32,7 +35,7 @@ function wireDecorator(
);
}
}
return createTrackedPropertyDescriptor(
return createWiredPropertyDescriptor(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

instead of using the tracked field for wired fields, we now have a different implementation.

);
}
// making the wired value as readonly
newValue = reactiveMembrane.getReadOnlyProxy(newValue);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@davidturissini this is the meat of it. Track uses reactiveMembrane.getProxy while this is now using reactiveMembrane.getReadOnlyProxy

@salesforce-best-lwc-internal
Copy link

Benchmark results

Base commit: 7b6773b | Target commit: 33337c2

lwc-engine-benchmark

table-append-1k metric base(7b6773b) target(33337c2) trend
benchmark-table/append/1k duration 139.60 (±4.80 ms) 139.70 (±5.00 ms) +0.1ms (0.1%) 👌
table-clear-1k metric base(7b6773b) target(33337c2) trend
benchmark-table/clear/1k duration 9.95 (±0.85 ms) 10.15 (±0.90 ms) +0.2ms (2.0%) 👌
table-create-10k metric base(7b6773b) target(33337c2) trend
benchmark-table/create/10k duration 834.65 (±5.70 ms) 848.60 (±6.35 ms) +14.0ms (1.7%) 👎
table-create-1k metric base(7b6773b) target(33337c2) trend
benchmark-table/create/1k duration 106.25 (±2.70 ms) 106.45 (±2.65 ms) +0.2ms (0.2%) 👌
table-update-10th-1k metric base(7b6773b) target(33337c2) trend
benchmark-table/update-10th/1k duration 75.25 (±4.85 ms) 75.75 (±5.25 ms) +0.5ms (0.7%) 👌
tablecmp-append-1k metric base(7b6773b) target(33337c2) trend
benchmark-table-component/append/1k duration 218.70 (±12.65 ms) 221.55 (±11.40 ms) +2.9ms (1.3%) 👌
tablecmp-clear-1k metric base(7b6773b) target(33337c2) trend
benchmark-table-component/clear/1k duration 6.05 (±1.00 ms) 6.05 (±0.85 ms) 0.0ms (0.0%) 👌
tablecmp-create-10k metric base(7b6773b) target(33337c2) trend
benchmark-table-component/create/10k duration 1626.30 (±21.35 ms) 1612.35 (±18.20 ms) -14.0ms (0.9%) 👍
tablecmp-create-1k metric base(7b6773b) target(33337c2) trend
benchmark-table-component/create/1k duration 184.60 (±3.35 ms) 184.50 (±4.40 ms) -0.1ms (0.1%) 👌
tablecmp-update-10th-1k metric base(7b6773b) target(33337c2) trend
benchmark-table-component/update-10th/1k duration 67.55 (±3.25 ms) 66.50 (±4.70 ms) -1.1ms (1.6%) 👌
wc-append-1k metric base(7b6773b) target(33337c2) trend
benchmark-table-wc/append/1k duration 229.40 (±9.00 ms) 228.90 (±9.00 ms) -0.5ms (0.2%) 👌
wc-clear-1k metric base(7b6773b) target(33337c2) trend
benchmark-table-wc/clear/1k duration 10.85 (±1.95 ms) 10.85 (±1.95 ms) 0.0ms (0.0%) 👌
wc-create-10k metric base(7b6773b) target(33337c2) trend
benchmark-table-wc/create/10k duration 1835.65 (±13.35 ms) 1831.95 (±13.30 ms) -3.7ms (0.2%) 👌
wc-create-1k metric base(7b6773b) target(33337c2) trend
benchmark-table-wc/create/1k duration 220.55 (±4.35 ms) 223.80 (±5.90 ms) +3.3ms (1.5%) 👎
wc-update-10th-1k metric base(7b6773b) target(33337c2) trend
benchmark-table-wc/update-10th/1k duration 67.50 (±3.95 ms) 66.00 (±4.55 ms) -1.5ms (2.2%) 👌

Copy link
Member

@pmdartus pmdartus left a comment

Choose a reason for hiding this comment

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

I don't understand the motivation of this breaking change, @caridy could you give more details?

@jbleyleSF
Copy link
Member

@caridy it sounds like there's a doc impact. Can you please explain this change with before and after code samples? Thanks in advance.

@caridy
Copy link
Contributor Author

caridy commented Jul 17, 2019

yes,. @jbleyleSF @pmdartus this is mostly experimental, to see where we land.

@caridy caridy added the nomerge label Aug 9, 2019
@caridy caridy changed the title refactor(engine): to provide readonly values for wired fields refactor(engine): implementation of wired reform Aug 12, 2019
@jodarove
Copy link
Contributor

Closed in favor of #1459

@jodarove jodarove closed this Sep 26, 2019
@jodarove jodarove deleted the caridy/wire-reform-1 branch September 26, 2019 05:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants