-
Notifications
You must be signed in to change notification settings - Fork 386
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): Locker Service related refactor #280
Conversation
Benchmark resultsBase commit: lwc-engine-benchmark
|
} | ||
} | ||
|
||
export default function api(): DecoratorFunction | any { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
api decorator factory
export type DecoratorFunction = (Ctor: any, key: PropertyKey, descriptor: PropertyDescriptor | undefined) => PropertyDescriptor; | ||
export type DecoratorMap = Record<string, DecoratorFunction>; | ||
|
||
export default function decorate(Ctor: any, decorators: DecoratorMap) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this file is inspired by Mobx (https://medium.com/@mweststrate/mobx-4-better-simpler-faster-smaller-c1fbc08008da), simplifying the application of decorators into classes with a simple transformation. It is limited, but it is all we need to make our decorators work for now.
|
||
// stub function to prevent misuse of the @track decorator | ||
export default function track(obj: any): any { | ||
export default function track(obj?: any): DecoratorFunction | any { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
track decorator factory
createTrackedPropertyDescriptor(proto, key, descriptor); | ||
|
||
// @wire is a factory that when invoked, returns the wire decorator | ||
export default function wire(adapter: any, config: any): DecoratorFunction { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wire decorator factory
createPublicPropertyDescriptor(proto, propName, descriptor); | ||
const decoratorMap: DecoratorMap = create(null); | ||
|
||
// TODO: eventually, the compiler should do this work |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this entire block is to keep backward compatibility. once we modify the compiler to decorate the class, we can remove this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kinda get what you're doing here, could you share some info about what babel or browser implementation of decorator would do to the decorate
api you've created? would it be replaced?
@yungcheng yeah, today this is mostly based on Mobx and ember, because decorators are ready yet, but eventually it will just be replaced with the babel implementation. |
Benchmark resultsBase commit: lwc-engine-benchmark
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR lack tests for the newly introduced decorate
method.
export type DecoratorFunction = (Ctor: any, key: PropertyKey, descriptor: PropertyDescriptor | undefined) => PropertyDescriptor; | ||
export type DecoratorMap = Record<string, DecoratorFunction>; | ||
|
||
export default function decorate(Ctor: any, decorators: DecoratorMap) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@caridy What do you think of the decorate
returning the Ctor the same way than MobX is doing?
This modification would make the greatly simplify the future job of the transform. For example with the current signature, in some case, the transform will need to create a temporary identifier in the scope.
// Original
export default class {
@api foo;
}
// Transformed
const _tmp = class {}
decorate(_tmp, {
foo: api()
});
export default _tmp;
If the decorate
method returns the passed object the transform wouldn't need to do this.
export default decorate(
class {},
{
foo: api()
}
);
export type DecoratorMap = Record<string, DecoratorFunction>; | ||
|
||
export default function decorate(Ctor: any, decorators: DecoratorMap) { | ||
if (process.env.NODE_ENV !== 'production') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since decorate
should become a public API, it would be better to do those checks regardless of the mode. The same way we do for createElement
for example.
if (process.env.NODE_ENV !== 'production') { | ||
assert.isTrue(isFunction(decorator), `decorate() expects a decorator function for "${prop}".`); | ||
} | ||
const target = isFunction(Ctor) ? Ctor.prototype : Ctor; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
target
is a loop invariant. It can get extracted from the loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not even needed, since line 9, the code asserts that Ctor
is a function.
const target = isFunction(Ctor) ? Ctor.prototype : Ctor; | ||
const originalDescriptor = getOwnPropertyDescriptor(target, prop); | ||
const descriptor = decorator(Ctor, prop, originalDescriptor); | ||
if (descriptor) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (descriptor) {
--> if (!isUndefined(descriptor)) {
@@ -24,8 +49,8 @@ export function prepareForPropUpdate(vm: VM) { | |||
} | |||
|
|||
// TODO: how to allow symbols as property keys? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can now remove the TODO. 💃
@@ -24,8 +49,8 @@ export function prepareForPropUpdate(vm: VM) { | |||
} | |||
|
|||
// TODO: how to allow symbols as property keys? | |||
export function createPublicPropertyDescriptor(proto: object, key: string, descriptor: PropertyDescriptor | undefined) { | |||
defineProperty(proto, key, { | |||
export function createPublicPropertyDescriptor(proto: object, key: PropertyKey, descriptor: PropertyDescriptor | undefined) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add return PropertyDescriptor
type.
} | ||
|
||
export function createPublicAccessorDescriptor(proto: object, key: string, descriptor: PropertyDescriptor) { | ||
export function createPublicAccessorDescriptor(Ctor: any, key: PropertyKey, descriptor: PropertyDescriptor) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add return PropertyDescriptor
type.
// TODO: how to allow symbols as property keys? | ||
export function createTrackedPropertyDescriptor(proto: object, key: string, descriptor: PropertyDescriptor | undefined) { | ||
defineProperty(proto, key, { | ||
export function createTrackedPropertyDescriptor(Ctor: any, key: PropertyKey, enumerable: boolean) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add return PropertyDescriptor
type.
assert.isTrue(writable !== false, `Compiler Error: A @track decorator can only be applied to a writable property.`); | ||
} | ||
} | ||
return createTrackedPropertyDescriptor(target, prop, isObject(descriptor) ? descriptor.enumerable === true : true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not passing down the PropertyDescriptor | undefined
like in createPublicPropertyDescriptor
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, those don't need the definition (at least not for now), they just need the enumerable. But if you feel strong about it, I can change it.
const { track } = getComponentDef(Ctor); | ||
if ('state' in component && (!track || !track.state)) { | ||
assert.logWarning(`Non-trackable component state detected in ${component}. Updates to state property will not be reactive. To make state reactive, add @track decorator.`); | ||
if ('state' in (vm.component as Component) && (!track || !track.state)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need this warning anymore. This was added when we removed implicit tracking to this.state
. Its been long enough now, that I think we can remove this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can remove this in another PR: #295
} | ||
// if it is configured as an accessor it must have a descriptor | ||
return createPublicAccessorDescriptor(target, propName, descriptor as PropertyDescriptor); | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't need else
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you explain more? I don't get this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
He is saying you are doing the same in the if and in the else, so you can put it out
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, it is not the same, we are calling two different methods, look closer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm saying that else
is not needed because we return inside of the if
block (line 38)
// linking elm and its component with VM | ||
component[ViewModelReflection] = elm[ViewModelReflection] = vm; | ||
defineProperties(elm, def.descriptors); | ||
function LWCElement(this: Component) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did we switch from class
to function
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because class constructor can't be invoked without new
, which is needed by locker.
Benchmark resultsBase commit: lwc-engine-benchmark
|
@pmdartus @davidturissini I have updated the PR based on the reviews, added tests. @diervo I still have to fold those decorators from factories to just decorators, will do it tomorrow. Numbers are looking really good. |
Benchmark resultsBase commit: lwc-engine-benchmark
|
Benchmark resultsBase commit: lwc-engine-benchmark
|
it('should throw when invoking it with no arguments', () => { | ||
expect(() => { | ||
decorate(); | ||
}).toThrow(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should validate a specific error message to eliminate the possibility of other failures inside 'decorate()' method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we are testing the decorate
function, it doesn't invoke anything since no arguments are provided.
class f {} | ||
expect(decorate(f, {})).toBe(f); | ||
}); | ||
it('should throw when invoking it with no arguments', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test title isn't clear since the decorator is invoked with paremeters
const COMPUTED_GETTER_MASK = 1; | ||
const COMPUTED_SETTER_MASK = 2; | ||
|
||
export default function api(target: any, propName: PropertyKey, descriptor: PropertyDescriptor | undefined): PropertyDescriptor { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would be nice to create Target type so that expected attributes can be easily validated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hard to define a class..., we can decorate any class.
} | ||
} | ||
const meta = target.publicProps; | ||
const config = (hasOwnProperty.call(target, 'publicProps') && hasOwnProperty.call(meta, propName)) ? meta[propName].config : 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can use meta !== undefined as a first condition since it has already been retrieved on line #20?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, this is testing that what we are retrieving is in fact an own prop, otherwise it is coming from inheritance, which is not the intent here, I can add a comment.
export type DecoratorMap = Record<string, DecoratorFunction>; | ||
|
||
export default function decorate(Ctor: any, decorators: DecoratorMap): any { | ||
if (!isFunction(Ctor) || decorators == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't second condition be triple equals? Otherwise we need to change the parameter type to include ' | undefined'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
intentionally ==, will add a comment
import { isRendering, vmBeingRendered } from "../invoker"; | ||
import { observeMutation, notifyMutation } from "../watcher"; | ||
import { VMElement } from "../vm"; | ||
import { getCustomElementVM } from "../html-element"; | ||
import { reactiveMembrane } from '../membrane'; | ||
|
||
// stub function to prevent misuse of the @track decorator | ||
export default function track(obj: any): any { | ||
export default function track(target: any, prop: PropertyKey, descriptor: PropertyDescriptor | undefined): PropertyDescriptor | any { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to be more explicit on return type, instead of 'any'?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is literally any because this method is overloaded, so you can call it with anything that it will wrap with a proxy, or u can use it as a decorator. I can probably improve that using TS overloading def.
} | ||
if (!isUndefined(descriptor)) { | ||
const { get, set, configurable, writable } = descriptor; | ||
assert.isTrue(!get && !set, `Compiler Error: A @track decorator can only be applied to a public field.`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should check for 'get' existence if 'set' is specified
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, we are testing that none of them exists, track can't have getter or setter.
export default function wire(adapter: any, config: any): DecoratorFunction { | ||
const len = arguments.length; | ||
if (len > 0 && len < 3) { | ||
return wireDecorator; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we care to check for undefined values?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, at the moment, we don't do anything with those just yet.
export interface MethodDef { | ||
[key: string]: 1; | ||
[key: string]: PublicMethod; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thumbs up on adding a type
if (wire) { | ||
for (const propName in wire) { | ||
const wireDef: WireDef = wire[propName]; | ||
if (wireDef.method) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need to check 'method' type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, this is not userland code, just compiled code. method there is a number 1 always.
Benchmark resultsBase commit: lwc-engine-benchmark
|
Benchmark resultsBase commit: lwc-engine-benchmark
|
const component = getCustomElementComponent(this); | ||
return component[key].apply(component, ArraySlice.call(arguments)); | ||
const vm = getCustomElementVM(this); | ||
return method.apply(vm.component, ArraySlice.call(arguments)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apply with arguments... review this.
Benchmark resultsBase commit: lwc-engine-benchmark
|
@davidturissini numbers are looking good! this is waiting for your review. I already did a first pass with @diervo, and @apapko did another pass. |
vm.component = component; | ||
// interaction hooks | ||
if (arguments.length === 1) { | ||
const { callHook, setHook, getHook } = arguments[0] as { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lift the type definition out of the function body.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And change the function signature to:
function LWCElement(this: Component, hooks?: ComponentHooks) {}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, intentionally not exposing this behavior so users don't need to know about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO there 2 aspects here, the type completeness and the APIs we want to advertise to customers:
- We should aim to have 100% type coverage in the codebase and make sure that even internal APIs are typed properly.
- While for SFDC customers we provide a set of blessed APIs to be used on the platform. This type definition file should be hand-crafted so this way we have control of what we expose or not.
const props = getOwnPropertyNames(decorators); | ||
// intentionally allowing decoration of classes only for now | ||
const target = Ctor.prototype; | ||
for (let i = 0, len = props.length; i < len; i += 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cache length
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is caching the length.
const target = Ctor.prototype; | ||
for (let i = 0, len = props.length; i < len; i += 1) { | ||
const prop = props[i]; | ||
const decorator = decorators[prop]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think propName
is better variable name here
// stub function to prevent misuse of the @track decorator | ||
export default function track(obj: any): any { | ||
export default function track(target: ComponentConstructor, prop: PropertyKey, descriptor: PropertyDescriptor | undefined): PropertyDescriptor; | ||
export default function track(target: any, prop?, descriptor?): any { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure optional types is what we want here. Perhaps prop: PropertyKey | undefined, descriptor: PropertyDescriptor | undefined
is better but I'm not sure.
I'm also not sure how prop is optional
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we change it to this one to have more clarity! cc @apapko
vm.component = component; | ||
// interaction hooks | ||
if (arguments.length === 1) { | ||
const { callHook, setHook, getHook } = arguments[0] as { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not name this argument? This is confusing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we name the arg, we can put the type in the function arguments, not inline
@@ -59,6 +61,18 @@ export interface VM { | |||
let idx: number = 0; | |||
let uid: number = 0; | |||
|
|||
function callHook(cmp: Component | undefined, fn: (...args: any[]) => any, args?: any[]): any { | |||
return fn.apply(cmp, args); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming we will add the actual hooks in another PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
locker pass those in... we have default behaviors here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets go
Benchmark resultsBase commit: lwc-engine-benchmark
|
This is a Locker Service related refactor.
Details
Follow up work
At the moment, the engine is still in charge of executing the decorators based on the metadata provided by the compilation step. e.g.:
Original Code:
Transpilation output today:
This should eventually be changed to:
Does this PR introduce a breaking change?