-
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
Changes from all commits
bbd7d77
7c7a4e0
d9b007c
e0fb827
19cbdcd
2988fc0
3c8db26
d7c951e
765ccb9
239837c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. we are testing the |
||
}); | ||
|
||
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 decorate a class', () => { | ||
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 |
---|---|---|
@@ -1,17 +1,43 @@ | ||
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 { Component, ComponentConstructor } 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: ComponentConstructor, propName: PropertyKey, descriptor: PropertyDescriptor | undefined): PropertyDescriptor { | ||
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; | ||
// publicProps must be an own property, otherwise the meta is inherited. | ||
const config = (!isUndefined(meta) && hasOwnProperty.call(target, 'publicProps') && hasOwnProperty.call(meta, propName)) ? meta[propName].config : 0; | ||
// 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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't need There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. I'm saying that |
||
return createPublicPropertyDescriptor(target, propName, descriptor); | ||
} | ||
} | ||
|
||
|
@@ -23,9 +49,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: ComponentConstructor, key: PropertyKey, descriptor: PropertyDescriptor | undefined): PropertyDescriptor { | ||
return { | ||
get(this: Component): any { | ||
const vm = getCustomElementVM(this); | ||
if (process.env.NODE_ENV !== 'production') { | ||
|
@@ -74,18 +99,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: ComponentConstructor, 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); | ||
|
@@ -125,5 +150,5 @@ export function createPublicAccessorDescriptor(proto: object, key: string, descr | |
} | ||
}, | ||
enumerable, | ||
}); | ||
}; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
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 { | ||
// intentionally comparing decorators with null and undefined | ||
if (!isFunction(Ctor) || decorators == null) { | ||
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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. this is caching the length. |
||
const propName = props[i]; | ||
const decorator = decorators[propName]; | ||
if (!isFunction(decorator)) { | ||
throw new TypeError(); | ||
} | ||
const originalDescriptor = getOwnPropertyDescriptor(target, propName); | ||
const descriptor = decorator(Ctor, propName, originalDescriptor); | ||
if (!isUndefined(descriptor)) { | ||
defineProperty(target, propName, descriptor); | ||
} | ||
} | ||
return Ctor; // chaining | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,24 +1,33 @@ | ||
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'; | ||
import { ComponentConstructor } from "../component"; | ||
|
||
// 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 commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure optional types is what we want here. Perhaps I'm also not sure how prop is optional There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we change it to this one to have more clarity! cc @apapko |
||
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.`); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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, isUndefined(descriptor) ? true : descriptor.enumerable === true); | ||
} | ||
|
||
// 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') { | ||
|
@@ -50,7 +59,7 @@ export function createTrackedPropertyDescriptor(proto: object, key: string, desc | |
} | ||
} | ||
}, | ||
enumerable: isUndefined(descriptor) ? true : descriptor.enumerable, | ||
configurable: false, | ||
}); | ||
enumerable, | ||
configurable: 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.
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