refactor: hidden fields instead of internal fields #1478
Conversation
|
So, I'm fine with this feature, but I think this will pose a perf regression, lets wait for the perf numbers. |
| @@ -4,7 +4,7 @@ | |||
| * SPDX-License-Identifier: MIT | |||
| * For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/MIT | |||
| */ | |||
| import { defineProperty } from './language'; | |||
| import { create, isUndefined } from './language'; | |||
There was a problem hiding this comment.
unrelated to this pr: The fields util is replicated in 3 submodules(engine, synthetic-shadow, node-reactions). Is it a good time to pull it out as a shared library or submodule?
cc @caridy
There was a problem hiding this comment.
yes, we have an issue for that
| } | ||
| valuesByField[fieldName] = value; | ||
| } | ||
| : setInternalField; // Fall back to symbol based approach in compat mode |
There was a problem hiding this comment.
@ekashida #860 (comment)
To give your some context as to why we were falling back to symbol based approach in compat mode
There was a problem hiding this comment.
Thanks. I didn't get the existing comment because it was actually falling back to the internal field approach (which was also based on symbols), which would fall back to the string-based approach.
|
@caridy @ravijayaramappa There were no comments from best, does that mean there was no perf regression? Also, any ideas why the perf tests require the vm to be set internally in vm.ts? |
| @@ -34,7 +34,7 @@ import { | |||
| getComponentAsString, | |||
| getTemplateReactiveObserver, | |||
| } from './component'; | |||
| import { setInternalField, setHiddenField } from '../shared/fields'; | |||
| import { setHiddenField, setInternalField } from '../shared/fields'; | |||
There was a problem hiding this comment.
where is getInternalField used? I thought the idea was to remove that entirely.
There was a problem hiding this comment.
Sorry, I mentioned above that it was in vm.ts but it was actually in def.ts:
/**
* EXPERIMENTAL: This function provides access to the component constructor,
* given an HTMLElement. This API is subject to change or being removed.
*/
export function getComponentConstructor(elm: HTMLElement): ComponentConstructor | null {
let ctor: ComponentConstructor | null = null;
if (elm instanceof HTMLElement) {
const vm = getInternalField(elm, ViewModelReflection);
if (!isUndefined(vm)) {
ctor = vm.def.ctor;
}
}
return ctor;
}
I'm not sure why, but our perf tests blow up when we access the vm using getHiddenField here.
|
@ekashida I think BEST summary is posted only if there is a significant regression. |
|
No @ravijayaramappa it is indeed failing, I have tried in another PR: |
Details
Fixes #1299
Does this PR introduce breaking changes?
No, it does not introduce breaking changes.