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): Locker Service related refactor #280

Merged
merged 10 commits into from
May 15, 2018
17 changes: 7 additions & 10 deletions packages/lwc-engine/src/framework/__tests__/def.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,7 @@ describe('def', () => {
bar() {}
}
MyComponent.publicMethods = ['foo', 'bar'];
expect(target.getComponentDef(MyComponent).methods).toEqual({
foo: 1,
bar: 1,
});
expect(Object.keys(target.getComponentDef(MyComponent).methods)).toEqual(['foo', 'bar']);
});

it('should understand static wire', () => {
Expand Down Expand Up @@ -255,12 +252,12 @@ describe('def', () => {

MySubComponent.publicMethods = ['fizz', 'buzz'];

expect(target.getComponentDef(MySubComponent).methods).toEqual({
foo: 1,
bar: 1,
fizz: 1,
buzz: 1
});
expect(Object.keys(target.getComponentDef(MySubComponent).methods)).toEqual([
'foo',
'bar',
'fizz',
'buzz'
]);
});

it('should inherit static wire properly', () => {
Expand Down
4 changes: 2 additions & 2 deletions packages/lwc-engine/src/framework/api.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import assert from "./assert";
import { freeze, isArray, isUndefined, isNull, isFunction, isObject, isString, ArrayPush, assign, create, forEach, StringSlice, StringCharCodeAt, isNumber } from "./language";
import { freeze, isArray, isUndefined, isNull, isFunction, isObject, isString, ArrayPush, assign, create, forEach, StringSlice, StringCharCodeAt, isNumber, hasOwnProperty } from "./language";
import { vmBeingRendered, invokeComponentCallback } from "./invoker";
import { EmptyArray, SPACE_CHAR } from "./utils";
import { renderVM, createVM, appendVM, removeVM, VM } from "./vm";
Expand Down Expand Up @@ -181,7 +181,7 @@ export function c(sel: string, Ctor: ComponentConstructor, data: VNodeData): VEl
// The compiler produce AMD modules that do not support circular dependencies
// We need to create an indirection to circumvent those cases.
// We could potentially move this check to the definition
if (Ctor.__circular__) {
if (hasOwnProperty.call(Ctor, '__circular__')) {
Ctor = Ctor();
}

Expand Down
12 changes: 6 additions & 6 deletions packages/lwc-engine/src/framework/component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import {
isRendering,
vmBeingRendered,
} from "./invoker";
import { isArray, ArrayIndexOf, ArraySplice } from "./language";
import { isArray, ArrayIndexOf, ArraySplice, isObject } from "./language";
import { invokeServiceHook, Services } from "./services";
import { getComponentDef, PropsDef, WireHash, TrackDef, ViewModelReflection } from './def';
import { VM } from "./vm";
Expand All @@ -19,7 +19,7 @@ export interface Component {
[ViewModelReflection]: VM;
readonly classList: DOMTokenList;
readonly root: ShadowRoot;
render?: () => void | Template;
render?: () => (void | Template);
connectedCallback?: () => void;
disconnectedCallback?: () => void;
renderedCallback?: () => void;
Expand Down Expand Up @@ -48,13 +48,13 @@ export function createComponent(vm: VM, Ctor: ComponentConstructor) {
assert.vm(vm);
}
// create the component instance
const component = invokeComponentConstructor(vm, Ctor);
invokeComponentConstructor(vm, Ctor);

if (process.env.NODE_ENV !== 'production') {
assert.isTrue(vm.component === component, `Invalid construction for ${vm}, maybe you are missing the call to super() on classes extending Element.`);
assert.isTrue(isObject(vm.component), `Invalid construction for ${vm}, maybe you are missing the call to super() on classes extending Element.`);
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)) {
Copy link
Contributor

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.

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 can remove this in another PR: #295

assert.logWarning(`Non-trackable component state detected in ${vm.component}. Updates to state property will not be reactive. To make state reactive, add @track decorator.`);
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -363,18 +363,4 @@ describe('decorators/api.ts', () => {
});
});

describe('@api misuse', () => {
it('should throw when invoking api as a function', () => {
class MyComponent extends Element {
constructor() {
super();
api();
}
}
expect(() => {
createElement('x-foo', { is: MyComponent });
}).toThrow('@api may only be used as a decorator.');
});
});

});
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
import decorate from "../decorate";

describe('decorate.ts', () => {
describe('decorate() api', () => {
it('should throw when invoking it with no arguments', () => {
expect(() => {
decorate();
}).toThrow();
Copy link
Collaborator

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?

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 are testing the decorate function, it doesn't invoke anything since no arguments are provided.

});

it('should throw when invoking it with a non-function', () => {
expect(() => {
decorate({}, {});
}).toThrow();
});

it('should throw when invoking it with an invalid decorator descriptor', () => {
expect(() => {
class f {}
decorate(f, undefined);
}).toThrow();
});

it('should throw when passed an invalid decorator', () => {
expect(() => {
class f {}
decorate(f, {
x: undefined
});
}).toThrow();
});
});
describe('decorate()', () => {
it('can be chained', () => {
class f {}
expect(decorate(f, {})).toBe(f);
});
it('should throw when invoking it with no arguments', () => {
Copy link
Collaborator

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

expect.assertions(7);
class f {
get y() { return 1; }
set y(v) { }
}
decorate(f, {
x: function (Ctor, key, descriptor) {
expect(Ctor).toBe(f);
expect(key).toBe('x');
expect(descriptor).toBe(undefined);
return {
value: 1
};
},
y: function (Ctor, key, descriptor) {
expect(Ctor).toBe(f);
expect(key).toBe('y');
expect(descriptor.configurable).toBe(true);
return descriptor;
}
});
expect(new f().x).toBe(1);
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -248,12 +248,6 @@ describe('track.ts', () => {
});

describe('track()', () => {
it('should throw when invoking it with no arguments', () => {
expect(() => {
track();
}).toThrow();
});

it('should throw when invoking it with more than one argument', () => {
expect(() => {
track({}, {});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@ describe('wire.ts', () => {
});

describe('@wire misuse', () => {
it('should throw when invoking wire as a function', () => {
it('should throw when invoking wire without adapter', () => {
class MyComponent extends Element {
constructor() {
super();
Expand All @@ -209,7 +209,7 @@ describe('wire.ts', () => {
}
expect(() => {
createElement('x-foo', { is: MyComponent });
}).toThrow('@wire may only be used as a decorator.');
}).toThrow('@wire(adapter, config?) may only be used as a decorator.');
});
});

Expand Down
48 changes: 36 additions & 12 deletions packages/lwc-engine/src/framework/decorators/api.ts
Original file line number Diff line number Diff line change
@@ -1,17 +1,42 @@
import assert from "../assert";
import { defineProperty, isObject, isNull, isTrue } from "../language";
import { isRendering, vmBeingRendered, isBeingConstructed } from "../invoker";
import { isObject, isNull, isTrue, hasOwnProperty } from "../language";
import { observeMutation, notifyMutation } from "../watcher";
import { Component } from "../component";
import { VM } from "../vm";
import { getCustomElementVM } from "../html-element";
import { isUndefined, isFunction } from "../language";
import { reactiveMembrane } from "../membrane";

// stub function to prevent misuse of the @api decorator
export default function api() {
const COMPUTED_GETTER_MASK = 1;
const COMPUTED_SETTER_MASK = 2;

export default function api(target: any, propName: PropertyKey, descriptor: PropertyDescriptor | undefined): PropertyDescriptor {
Copy link
Collaborator

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

Copy link
Contributor Author

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.

if (process.env.NODE_ENV !== 'production') {
assert.fail("@api may only be used as a decorator.");
if (arguments.length !== 3) {
assert.fail(`@api decorator can only be used as a decorator function.`);
}
}
const meta = target.publicProps;
const config = (hasOwnProperty.call(target, 'publicProps') && hasOwnProperty.call(meta, propName)) ? meta[propName].config : 0;
Copy link
Collaborator

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?

Copy link
Contributor Author

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.

// initializing getters and setters for each public prop on the target prototype
if (COMPUTED_SETTER_MASK & config || COMPUTED_GETTER_MASK & config) {
if (process.env.NODE_ENV !== 'production') {
assert.invariant(!descriptor || (isFunction(descriptor.get) || isFunction(descriptor.set)), `Invalid property ${propName} definition in ${target}, it cannot be a prototype definition if it is a public property. Instead use the constructor to define it.`);
const mustHaveGetter = COMPUTED_GETTER_MASK & config;
const mustHaveSetter = COMPUTED_SETTER_MASK & config;
if (mustHaveGetter) {
assert.isTrue(isObject(descriptor) && isFunction(descriptor.get), `Missing getter for property ${propName} decorated with @api in ${target}`);
}
if (mustHaveSetter) {
assert.isTrue(isObject(descriptor) && isFunction(descriptor.set), `Missing setter for property ${propName} decorated with @api in ${target}`);
assert.isTrue(mustHaveGetter, `Missing getter for property ${propName} decorated with @api in ${target}. You cannot have a setter without the corresponding getter.`);
}
}
// if it is configured as an accessor it must have a descriptor
return createPublicAccessorDescriptor(target, propName, descriptor as PropertyDescriptor);
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't need else

Copy link
Contributor Author

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.

Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Contributor

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)

return createPublicPropertyDescriptor(target, propName, descriptor);
}
}

Expand All @@ -23,9 +48,8 @@ export function prepareForPropUpdate(vm: VM) {
vmBeingUpdated = 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): PropertyDescriptor {
return {
get(this: Component): any {
const vm = getCustomElementVM(this);
if (process.env.NODE_ENV !== 'production') {
Expand Down Expand Up @@ -74,18 +98,18 @@ export function createPublicPropertyDescriptor(proto: object, key: string, descr
}
},
enumerable: isUndefined(descriptor) ? true : descriptor.enumerable,
});
};
}

export function createPublicAccessorDescriptor(proto: object, key: string, descriptor: PropertyDescriptor) {
export function createPublicAccessorDescriptor(Ctor: any, key: PropertyKey, descriptor: PropertyDescriptor): PropertyDescriptor {
const { get, set, enumerable } = descriptor;
if (!isFunction(get)) {
if (process.env.NODE_ENV !== 'production') {
assert.fail(`Invalid attempt to create public property descriptor ${key} in ${proto}. It is missing the getter declaration with @api get ${key}() {} syntax.`);
assert.fail(`Invalid attempt to create public property descriptor ${key} in ${Ctor}. It is missing the getter declaration with @api get ${key}() {} syntax.`);
}
throw new TypeError();
}
defineProperty(proto, key, {
return {
get(this: Component): any {
if (process.env.NODE_ENV !== 'production') {
const vm = getCustomElementVM(this);
Expand Down Expand Up @@ -125,5 +149,5 @@ export function createPublicAccessorDescriptor(proto: object, key: string, descr
}
},
enumerable,
});
};
}
26 changes: 26 additions & 0 deletions packages/lwc-engine/src/framework/decorators/decorate.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
import { getOwnPropertyNames, getOwnPropertyDescriptor, defineProperty, isFunction, isUndefined } from "../language";

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): any {
if (!isFunction(Ctor) || decorators == null) {
Copy link
Collaborator

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'

Copy link
Contributor Author

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

throw new TypeError();
}
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) {
Copy link
Contributor

@davidturissini davidturissini May 15, 2018

Choose a reason for hiding this comment

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

cache length

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 caching the length.

const prop = props[i];
const decorator = decorators[prop];
Copy link
Contributor

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

if (!isFunction(decorator)) {
throw new TypeError();
}
const originalDescriptor = getOwnPropertyDescriptor(target, prop);
const descriptor = decorator(Ctor, prop, originalDescriptor);
if (!isUndefined(descriptor)) {
defineProperty(target, prop, descriptor);
}
}
return Ctor; // chaining
}
31 changes: 19 additions & 12 deletions packages/lwc-engine/src/framework/decorators/track.ts
Original file line number Diff line number Diff line change
@@ -1,24 +1,31 @@
import assert from "../assert";
import { isArray, isObject, defineProperty, isUndefined } from "../language";
import { isArray, isObject, isUndefined } from "../language";
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 {
Copy link
Collaborator

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'?

Copy link
Contributor Author

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 (arguments.length === 1) {
return reactiveMembrane.getProxy(target);
}
if (process.env.NODE_ENV !== 'production') {
if (arguments.length !== 1) {
assert.fail("@track can be used as a decorator or as a function with one argument to produce a trackable version of the provided value.");
if (arguments.length !== 3) {
assert.fail(`@track decorator can only be used with one argument to return a trackable object, or as a decorator function.`);
}
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.`);
Copy link
Collaborator

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

Copy link
Contributor Author

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.

assert.isTrue(configurable !== false, `Compiler Error: A @track decorator can only be applied to a configurable property.`);
assert.isTrue(writable !== false, `Compiler Error: A @track decorator can only be applied to a writable property.`);
}
}
return reactiveMembrane.getProxy(obj);
return createTrackedPropertyDescriptor(target, prop, isObject(descriptor) ? descriptor.enumerable === true : true);
Copy link
Member

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.

Copy link
Contributor Author

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.

}

// 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): PropertyDescriptor {
return {
get(this: VMElement): any {
const vm = getCustomElementVM(this);
if (process.env.NODE_ENV !== 'production') {
Expand Down Expand Up @@ -50,7 +57,7 @@ export function createTrackedPropertyDescriptor(proto: object, key: string, desc
}
}
},
enumerable: isUndefined(descriptor) ? true : descriptor.enumerable,
configurable: false,
});
enumerable,
configurable: true,
};
}
Loading