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

[BUGFIX beta] Make assertions from on and fn more actionable. #18871

Merged
merged 1 commit into from
Apr 14, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 18 additions & 4 deletions packages/@ember/-internals/glimmer/lib/helpers/fn.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { assert } from '@ember/debug';
import { DEBUG } from '@glimmer/env';
import { CapturedArguments, VM, VMArguments } from '@glimmer/interfaces';
import { CapturedArguments, Environment, VM, VMArguments } from '@glimmer/interfaces';
import { HelperRootReference } from '@glimmer/reference';
import buildUntouchableThis from '../utils/untouchable-this';
import { INVOKE } from './mut';
Expand Down Expand Up @@ -79,14 +79,21 @@ const context = buildUntouchableThis('`fn` helper');
@since 3.11.0
*/

function fn({ positional }: CapturedArguments) {
function fn({ positional }: CapturedArguments, env?: Environment<unknown>) {
let callbackRef = positional.at(0);

assert(
`You must pass a function as the \`fn\` helpers first argument.`,
callbackRef !== undefined
);

if (DEBUG && typeof callbackRef[INVOKE] !== 'function') {
let callback = callbackRef.value();

assert(
`You must pass a function as the \`fn\` helpers first argument, you passed ${callback}`,
`You must pass a function as the \`fn\` helpers first argument, you passed ${
callback === null ? 'null' : typeof callback
}. ${env!.getTemplatePathDebugContext(callbackRef)}`,
typeof callback === 'function'
);
}
Expand All @@ -105,5 +112,12 @@ function fn({ positional }: CapturedArguments) {
}

export default function(args: VMArguments, vm: VM) {
return new HelperRootReference(fn, args.capture(), vm.env);
let callback = fn;
if (DEBUG) {
callback = (args: CapturedArguments) => {
return fn(args, vm.env);
};
}

return new HelperRootReference(callback, args.capture(), vm.env);
}
43 changes: 35 additions & 8 deletions packages/@ember/-internals/glimmer/lib/modifiers/on.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
import { Owner } from '@ember/-internals/owner';
import { assert } from '@ember/debug';
import { DEBUG } from '@glimmer/env';
import { CapturedArguments, Destroyable, ModifierManager, VMArguments } from '@glimmer/interfaces';
import { expect } from '@glimmer/util';
import { CONSTANT_TAG, Tag } from '@glimmer/validator';
import { SimpleElement } from '@simple-dom/interface';
import { Renderer } from '../renderer';
import buildUntouchableThis from '../utils/untouchable-this';

const untouchableContext = buildUntouchableThis('`on` modifier');
Expand Down Expand Up @@ -48,6 +51,7 @@ const SUPPORTS_EVENT_OPTIONS = (() => {

export class OnModifierState {
public tag: Tag;
public owner: Owner;
public element: Element;
public args: CapturedArguments;
public eventName!: string;
Expand All @@ -59,7 +63,8 @@ export class OnModifierState {
public options?: AddEventListenerOptions;
public shouldUpdate = true;

constructor(element: Element, args: CapturedArguments) {
constructor(owner: Owner, element: Element, args: CapturedArguments) {
this.owner = owner;
this.element = element;
this.args = args;
this.tag = args.tag;
Expand Down Expand Up @@ -101,11 +106,31 @@ export class OnModifierState {
this.shouldUpdate = true;
}

assert(
'You must pass a function as the second argument to the `on` modifier',
args.positional.at(1) !== undefined && typeof args.positional.at(1).value() === 'function'
);
let userProvidedCallback = args.positional.at(1).value() as EventListener;
let userProvidedCallbackReference = args.positional.at(1);

if (DEBUG) {
assert(
`You must pass a function as the second argument to the \`on\` modifier.`,
args.positional.at(1) !== undefined
);

// hardcoding `renderer:-dom` here because we guard for `this.isInteractive` before instantiating OnModifierState, it can never be created when the renderer is `renderer:-inert`
let renderer = expect(
this.owner.lookup<Renderer>('renderer:-dom'),
`BUG: owner is missing renderer:-dom`
);
let stack = renderer.debugRenderTree.logRenderStackForPath(userProvidedCallbackReference);

let value = userProvidedCallbackReference.value();
assert(
`You must pass a function as the second argument to the \`on\` modifier, you passed ${
value === null ? 'null' : typeof value
}. While rendering:\n\n${stack}`,
typeof value === 'function'
);
}

let userProvidedCallback = userProvidedCallbackReference.value() as EventListener;
if (userProvidedCallback !== this.userProvidedCallback) {
this.userProvidedCallback = userProvidedCallback;
this.shouldUpdate = true;
Expand Down Expand Up @@ -303,9 +328,11 @@ function addEventListener(
export default class OnModifierManager implements ModifierManager<OnModifierState | null, unknown> {
public SUPPORTS_EVENT_OPTIONS: boolean = SUPPORTS_EVENT_OPTIONS;
public isInteractive: boolean;
private owner: Owner;

constructor(isInteractive: boolean) {
constructor(owner: Owner, isInteractive: boolean) {
this.isInteractive = isInteractive;
this.owner = owner;
}

get counters() {
Expand All @@ -319,7 +346,7 @@ export default class OnModifierManager implements ModifierManager<OnModifierStat

const capturedArgs = args.capture();

return new OnModifierState(<Element>element, capturedArgs);
return new OnModifierState(this.owner, <Element>element, capturedArgs);
}

getTag(state: OnModifierState | null): Tag {
Expand Down
2 changes: 1 addition & 1 deletion packages/@ember/-internals/glimmer/lib/renderer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,7 @@ export abstract class Renderer {
this._builder = builder;

// resolver is exposed for tests
let runtimeResolver = (this._runtimeResolver = new RuntimeResolver(env.isInteractive));
let runtimeResolver = (this._runtimeResolver = new RuntimeResolver(owner, env.isInteractive));
let compileTimeResolver = new CompileTimeResolver(runtimeResolver);

let context = (this._context = JitContext(compileTimeResolver));
Expand Down
4 changes: 2 additions & 2 deletions packages/@ember/-internals/glimmer/lib/resolver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -287,12 +287,12 @@ export default class RuntimeResolver implements JitRuntimeResolver<OwnedTemplate
public componentDefinitionCount = 0;
public helperDefinitionCount = 0;

constructor(isInteractive: boolean) {
constructor(owner: Owner, isInteractive: boolean) {
this.isInteractive = isInteractive;

this.builtInModifiers = {
action: { manager: new ActionModifierManager(), state: null },
on: { manager: new OnModifierManager(isInteractive), state: null },
on: { manager: new OnModifierManager(owner, isInteractive), state: null },
};
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -120,14 +120,34 @@ moduleFor(
assert.equal(this.stashedFn(), 'arg1: foo, arg2: bar');
}

'@test asserts if the first argument is not a function'() {
'@test asserts if no argument given'() {
expectAssertion(() => {
this.render(`{{invoke (fn)}}`, {
myFunc: null,
arg1: 'foo',
arg2: 'bar',
});
}, /You must pass a function as the `fn` helpers first argument./);
}

'@test asserts if the first argument is undefined'() {
expectAssertion(() => {
this.render(`{{invoke (fn this.myFunc this.arg1 this.arg2)}}`, {
myFunc: undefined,
arg1: 'foo',
arg2: 'bar',
});
}, /You must pass a function as the `fn` helpers first argument, you passed undefined. While rendering:\n\nthis.myFunc/);
}

'@test asserts if the first argument is null'() {
expectAssertion(() => {
this.render(`{{invoke (fn this.myFunc this.arg1 this.arg2)}}`, {
myFunc: null,
arg1: 'foo',
arg2: 'bar',
});
}, /You must pass a function as the `fn` helpers first argument, you passed null/);
}, /You must pass a function as the `fn` helpers first argument, you passed null. While rendering:\n\nthis.myFunc/);
}

'@test asserts if the provided function accesses `this` without being bound prior to passing to fn'(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -309,13 +309,13 @@ moduleFor(
'@test asserts when callback is undefined'() {
expectAssertion(() => {
this.render(`<button {{on 'click' this.foo}}>Click Me</button>`);
}, /You must pass a function as the second argument to the `on` modifier/);
}, /You must pass a function as the second argument to the `on` modifier, you passed undefined. While rendering:\n\nthis.foo/);
}

'@test asserts when callback is null'() {
expectAssertion(() => {
this.render(`<button {{on 'click' this.foo}}>Click Me</button>`, { foo: null });
}, /You must pass a function as the second argument to the `on` modifier/);
}, /You must pass a function as the second argument to the `on` modifier, you passed null. While rendering:\n\nthis.foo/);
}

'@test asserts if the provided callback accesses `this` without being bound prior to passing to on'(
Expand Down