From 6ed071de16e139ceca78e52e979b78b7fb29bfc1 Mon Sep 17 00:00:00 2001 From: Robert Jackson Date: Mon, 22 Apr 2019 10:41:02 -0400 Subject: [PATCH 1/2] [FEATURE] Implement `on` modifier. See https://emberjs.github.io/rfcs/0471-on-modifier.html for details. Special thanks to [buschtoens](https://github.com/buschtoens)'s work on [ember-on-modifier](https://github.com/buschtoens/ember-on-modifier) which heavily inspired this implementation. --- .../-internals/glimmer/lib/modifiers/on.ts | 241 +++++++++++++ .../@ember/-internals/glimmer/lib/resolver.ts | 15 +- .../tests/integration/modifiers/on-test.js | 324 ++++++++++++++++++ packages/@ember/canary-features/index.ts | 2 + 4 files changed, 578 insertions(+), 4 deletions(-) create mode 100644 packages/@ember/-internals/glimmer/lib/modifiers/on.ts create mode 100644 packages/@ember/-internals/glimmer/tests/integration/modifiers/on-test.js diff --git a/packages/@ember/-internals/glimmer/lib/modifiers/on.ts b/packages/@ember/-internals/glimmer/lib/modifiers/on.ts new file mode 100644 index 00000000000..04841666fe7 --- /dev/null +++ b/packages/@ember/-internals/glimmer/lib/modifiers/on.ts @@ -0,0 +1,241 @@ +import { assert } from '@ember/debug'; +import { DEBUG } from '@glimmer/env'; +import { Opaque, Simple } from '@glimmer/interfaces'; +import { Tag } from '@glimmer/reference'; +import { Arguments, CapturedArguments, ModifierManager } from '@glimmer/runtime'; +import { Destroyable } from '@glimmer/util'; + +/* + Internet Explorer 11 does not support `once` and also does not support + passing `eventOptions`. In some situations it then throws a weird script + error, like: + + ``` + Could not complete the operation due to error 80020101 + ``` + + This flag determines, whether `{ once: true }` and thus also event options in + general are supported. +*/ +const SUPPORTS_EVENT_OPTIONS = (() => { + try { + const div = document.createElement('div'); + let counter = 0; + div.addEventListener('click', () => counter++, { once: true }); + + let event; + if (typeof Event === 'function') { + event = new Event('click'); + } else { + event = document.createEvent('Event'); + event.initEvent('click', true, true); + } + + div.dispatchEvent(event); + div.dispatchEvent(event); + + return counter === 1; + } catch (error) { + return false; + } +})(); + +export class OnModifierState { + public tag: Tag; + public element: Element; + public args: CapturedArguments; + public eventName!: string; + public callback!: EventListener; + private userProvidedCallback!: EventListener; + public once?: boolean; + public passive?: boolean; + public capture?: boolean; + public options?: AddEventListenerOptions; + public shouldUpdate = true; + + constructor(element: Element, args: CapturedArguments) { + this.element = element; + this.args = args; + this.tag = args.tag; + } + + updateFromArgs() { + let { args } = this; + + let { once, passive, capture }: AddEventListenerOptions = args.named.value(); + if (once !== this.once) { + this.once = once; + this.shouldUpdate = true; + } + + if (passive !== this.passive) { + this.passive = passive; + this.shouldUpdate = true; + } + + if (capture !== this.capture) { + this.capture = capture; + this.shouldUpdate = true; + } + + let options: AddEventListenerOptions; + if (once || passive || capture) { + options = this.options = { once, passive, capture }; + } else { + this.options = undefined; + } + + assert( + 'You must pass a valid DOM event name as the first argument to the `on` modifier', + args.positional.at(0) !== undefined && typeof args.positional.at(0).value() === 'string' + ); + let eventName = args.positional.at(0).value() as string; + if (eventName !== this.eventName) { + this.eventName = eventName; + 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; + if (userProvidedCallback !== this.userProvidedCallback) { + this.userProvidedCallback = userProvidedCallback; + this.shouldUpdate = true; + } + + if (this.shouldUpdate) { + let callback = (this.callback = function(this: Element, event) { + if (DEBUG && passive) { + event.preventDefault = () => { + assert( + `You marked this listener as 'passive', meaning that you must not call 'event.preventDefault()': \n\n${userProvidedCallback}` + ); + }; + } + + if (!SUPPORTS_EVENT_OPTIONS && once) { + removeEventListener(this, eventName, callback, options); + } + return userProvidedCallback.call(null, event); + }); + } + } + + destroy() { + let { element, eventName, callback, options } = this; + + removeEventListener(element, eventName, callback, options); + } +} + +let adds = 0; +let removes = 0; + +function removeEventListener( + element: Element, + eventName: string, + callback: EventListener, + options?: AddEventListenerOptions +): void { + removes++; + + if (SUPPORTS_EVENT_OPTIONS) { + // when options are supported, use them across the board + element.removeEventListener(eventName, callback, options); + } else if (options !== undefined && options.capture) { + // used only in the following case: + // + // `{ once: true | false, passive: true | false, capture: true } + // + // `once` is handled via a custom callback that removes after first + // invocation so we only care about capture here as a boolean + element.removeEventListener(eventName, callback, true); + } else { + // used only in the following cases: + // + // * where there is no options + // * `{ once: true | false, passive: true, capture: false } + element.removeEventListener(eventName, callback); + } +} + +function addEventListener( + element: Element, + eventName: string, + callback: EventListener, + options?: AddEventListenerOptions +): void { + adds++; + + if (SUPPORTS_EVENT_OPTIONS) { + // when options are supported, use them across the board + element.addEventListener(eventName, callback, options); + } else if (options !== undefined && options.capture) { + // used only in the following case: + // + // `{ once: true | false, passive: true | false, capture: true } + // + // `once` is handled via a custom callback that removes after first + // invocation so we only care about capture here as a boolean + element.addEventListener(eventName, callback, true); + } else { + // used only in the following cases: + // + // * where there is no options + // * `{ once: true | false, passive: true, capture: false } + element.addEventListener(eventName, callback); + } +} + +export default class OnModifierManager implements ModifierManager { + public SUPPORTS_EVENT_OPTIONS: boolean = SUPPORTS_EVENT_OPTIONS; + + get counters() { + return { adds, removes }; + } + + create(element: Simple.Element | Element, _state: Opaque, args: Arguments) { + const capturedArgs = args.capture(); + + return new OnModifierState(element, capturedArgs); + } + + getTag({ tag }: OnModifierState): Tag { + return tag; + } + + install(state: OnModifierState) { + state.updateFromArgs(); + + let { element, eventName, callback, options } = state; + + addEventListener(element, eventName, callback, options); + + state.shouldUpdate = false; + } + + update(state: OnModifierState) { + // stash prior state for el.removeEventListener + let { element, eventName, callback, options } = state; + + state.updateFromArgs(); + + if (!state.shouldUpdate) { + return; + } + + // use prior state values for removal + removeEventListener(element, eventName, callback, options); + + // read updated values from the state object + addEventListener(state.element, state.eventName, state.callback, state.options); + + state.shouldUpdate = false; + } + + getDestructor(state: Destroyable) { + return state; + } +} diff --git a/packages/@ember/-internals/glimmer/lib/resolver.ts b/packages/@ember/-internals/glimmer/lib/resolver.ts index 6c25171f12b..93a53d67ff2 100644 --- a/packages/@ember/-internals/glimmer/lib/resolver.ts +++ b/packages/@ember/-internals/glimmer/lib/resolver.ts @@ -5,6 +5,7 @@ import { lookupComponent, lookupPartial, OwnedTemplateMeta } from '@ember/-inter import { EMBER_GLIMMER_ANGLE_BRACKET_BUILT_INS, EMBER_GLIMMER_FN_HELPER, + EMBER_GLIMMER_ON_MODIFIER, EMBER_MODULE_UNIFICATION, } from '@ember/canary-features'; import { assert } from '@ember/debug'; @@ -44,6 +45,7 @@ import { default as readonly } from './helpers/readonly'; import { default as unbound } from './helpers/unbound'; import ActionModifierManager from './modifiers/action'; import { CustomModifierDefinition, ModifierManagerDelegate } from './modifiers/custom'; +import OnModifierManager from './modifiers/on'; import { populateMacros } from './syntax'; import { mountHelper } from './syntax/mount'; import { outletHelper } from './syntax/outlet'; @@ -95,10 +97,17 @@ if (EMBER_GLIMMER_FN_HELPER) { BUILTINS_HELPERS.fn = fn; } -const BUILTIN_MODIFIERS = { +interface IBuiltInModifiers { + [name: string]: ModifierDefinition | undefined; +} +const BUILTIN_MODIFIERS: IBuiltInModifiers = { action: { manager: new ActionModifierManager(), state: null }, + on: undefined, }; +if (EMBER_GLIMMER_ON_MODIFIER) { + BUILTIN_MODIFIERS.on = { manager: new OnModifierManager(), state: null }; +} export default class RuntimeResolver implements IRuntimeResolver { public compiler: LazyCompiler; @@ -109,9 +118,7 @@ export default class RuntimeResolver implements IRuntimeResolver> = new Map(); diff --git a/packages/@ember/-internals/glimmer/tests/integration/modifiers/on-test.js b/packages/@ember/-internals/glimmer/tests/integration/modifiers/on-test.js new file mode 100644 index 00000000000..bdc79a6eaf8 --- /dev/null +++ b/packages/@ember/-internals/glimmer/tests/integration/modifiers/on-test.js @@ -0,0 +1,324 @@ +import { EMBER_GLIMMER_ON_MODIFIER } from '@ember/canary-features'; +import { moduleFor, RenderingTestCase, runTask } from 'internal-test-helpers'; +import { isChrome, isFirefox } from '@ember/-internals/browser-environment'; +import { privatize as P } from '@ember/-internals/container'; + +const isIE11 = !window.ActiveXObject && 'ActiveXObject' in window; + +if (EMBER_GLIMMER_ON_MODIFIER) { + moduleFor( + '{{on}} Modifier', + class extends RenderingTestCase { + beforeEach() { + // might error if getOnManagerInstance fails + this.startingCounters = this.getOnManagerInstance().counters; + } + + getOnManagerInstance() { + // leveraging private APIs, this can be deleted if these APIs change + // but it has been useful to verify some internal details + let templateCompiler = this.owner.lookup(P`template-compiler:main`); + + return templateCompiler.resolver.resolver.builtInModifiers.on.manager; + } + + assertCounts(expected) { + let { counters } = this.getOnManagerInstance(); + + this.assert.deepEqual( + counters, + { + adds: expected.adds + this.startingCounters.adds, + removes: expected.removes + this.startingCounters.removes, + }, + `counters have incremented by ${JSON.stringify(expected)}` + ); + } + + '@test SUPPORTS_EVENT_OPTIONS is correct (private API usage)'(assert) { + let manager = this.getOnManagerInstance(); + + let { SUPPORTS_EVENT_OPTIONS } = manager; + + if (isChrome || isFirefox) { + assert.strictEqual(SUPPORTS_EVENT_OPTIONS, true, 'is true in chrome and firefox'); + } else if (isIE11) { + assert.strictEqual(SUPPORTS_EVENT_OPTIONS, false, 'is true in chrome and firefox'); + } else { + assert.expect(0); + } + } + + ['@test it adds an event listener'](assert) { + let count = 0; + + this.render('', { + callback() { + count++; + }, + }); + + assert.equal(count, 0, 'not called on initial render'); + + this.assertStableRerender(); + this.assertCounts({ adds: 1, removes: 0 }); + assert.equal(count, 0, 'not called on a rerender'); + + runTask(() => this.$('button').click()); + assert.equal(count, 1, 'has been called 1 time'); + + runTask(() => this.$('button').click()); + assert.equal(count, 2, 'has been called 2 times'); + + this.assertCounts({ adds: 1, removes: 0 }); + } + + '@test passes the event to the listener'(assert) { + let event; + this.render('', { + callback(evt) { + event = evt; + }, + }); + + runTask(() => this.$('button').click()); + assert.strictEqual( + event.target, + this.element.querySelector('button'), + 'has a valid event with a target' + ); + + this.assertCounts({ adds: 1, removes: 0 }); + } + + '@test the listener callback is bound'(assert) { + let first = 0; + let second = 0; + let firstCallback = () => first++; + let secondCallback = () => second++; + + this.render('', { + callback: firstCallback, + }); + + assert.equal(first, 0, 'precond - first not called on initial render'); + assert.equal(second, 0, 'precond - second not called on initial render'); + + runTask(() => this.$('button').click()); + assert.equal(first, 1, 'first has been called 1 time'); + assert.equal(second, 0, 'second not called on initial render'); + + runTask(() => this.context.set('callback', secondCallback)); + runTask(() => this.$('button').click()); + + assert.equal(first, 1, 'first has been called 1 time'); + assert.equal(second, 1, 'second has been called 1 times'); + + this.assertCounts({ adds: 2, removes: 1 }); + } + + '@test setting once named argument ensures the callback is only called once'(assert) { + let count = 0; + + this.render('', { + callback() { + count++; + }, + }); + + assert.equal(count, 0, 'not called on initial render'); + + this.assertStableRerender(); + assert.equal(count, 0, 'not called on a rerender'); + + runTask(() => this.$('button').click()); + assert.equal(count, 1, 'has been called 1 time'); + + runTask(() => this.$('button').click()); + assert.equal(count, 1, 'has been called 1 times'); + + if (isIE11) { + this.assertCounts({ adds: 1, removes: 1 }); + } else { + this.assertCounts({ adds: 1, removes: 0 }); + } + } + + '@test changing from `once=false` to `once=true` ensures the callback can only be called once'( + assert + ) { + let count = 0; + + this.render('', { + callback() { + count++; + }, + + once: false, + }); + + runTask(() => this.$('button').click()); + assert.equal(count, 1, 'has been called 1 time'); + + runTask(() => this.$('button').click()); + assert.equal(count, 2, 'has been called 2 times'); + + runTask(() => this.context.set('once', true)); + runTask(() => this.$('button').click()); + assert.equal(count, 3, 'has been called 3 time'); + + runTask(() => this.$('button').click()); + assert.equal(count, 3, 'is not called again'); + + if (isIE11) { + this.assertCounts({ adds: 2, removes: 2 }); + } else { + this.assertCounts({ adds: 2, removes: 1 }); + } + } + + '@test setting passive named argument prevents calling preventDefault'() { + let matcher = /You marked this listener as 'passive', meaning that you must not call 'event.preventDefault\(\)'/; + this.render('', { + callback(event) { + expectAssertion(() => { + event.preventDefault(); + }, matcher); + }, + }); + + runTask(() => this.$('button').click()); + } + + '@test by default bubbling is used (capture: false)'(assert) { + this.render( + ` +
+
+
+ `, + { + handleOuterClick() { + assert.step('outer clicked'); + }, + handleInnerClick() { + assert.step('inner clicked'); + }, + } + ); + + runTask(() => this.$('.inner').click()); + + assert.verifySteps(['inner clicked', 'outer clicked'], 'uses capture: false by default'); + } + + '@test specifying capture named argument uses capture semantics'(assert) { + this.render( + ` +
+
+
+ `, + { + handleOuterClick() { + assert.step('outer clicked'); + }, + handleInnerClick() { + assert.step('inner clicked'); + }, + } + ); + + runTask(() => this.$('.inner').click()); + + assert.verifySteps(['outer clicked', 'inner clicked'], 'capture works'); + } + + '@test can use capture and once together'(assert) { + this.render( + ` +
+
+
+ `, + { + handleOuterClick() { + assert.step('outer clicked'); + }, + handleInnerClick() { + assert.step('inner clicked'); + }, + } + ); + + runTask(() => this.$('.inner').click()); + + assert.verifySteps(['outer clicked', 'inner clicked'], 'capture works'); + + runTask(() => this.$('.inner').click()); + assert.verifySteps(['inner clicked'], 'once works'); + } + + '@test unrelated updates to `this` context does not result in removing + re-adding'(assert) { + let called = false; + + this.render('', { + callback() { + called = true; + }, + otherThing: 0, + }); + + this.assertCounts({ adds: 1, removes: 0 }); + + runTask(() => this.$('button').click()); + assert.equal(called, 1, 'callback is being invoked'); + + runTask(() => this.context.set('otherThing', 1)); + this.assertCounts({ adds: 1, removes: 0 }); + } + + '@test asserts when eventName is missing'() { + expectAssertion(() => { + this.render(``, { + callback() {}, + }); + }, /You must pass a valid DOM event name as the first argument to the `on` modifier/); + } + + '@test asserts when eventName is a bound undefined value'() { + expectAssertion(() => { + this.render(``, { + callback() {}, + }); + }, /You must pass a valid DOM event name as the first argument to the `on` modifier/); + } + + '@test asserts when eventName is a function'() { + expectAssertion(() => { + this.render(``, { + callback() {}, + }); + }, /You must pass a valid DOM event name as the first argument to the `on` modifier/); + } + + '@test asserts when callback is missing'() { + expectAssertion(() => { + this.render(``); + }, /You must pass a function as the second argument to the `on` modifier/); + } + + '@test asserts when callback is undefined'() { + expectAssertion(() => { + this.render(``); + }, /You must pass a function as the second argument to the `on` modifier/); + } + + '@test asserts when callback is null'() { + expectAssertion(() => { + this.render(``, { foo: null }); + }, /You must pass a function as the second argument to the `on` modifier/); + } + } + ); +} diff --git a/packages/@ember/canary-features/index.ts b/packages/@ember/canary-features/index.ts index 8dcf2f3268f..fcd9aa0eb6d 100644 --- a/packages/@ember/canary-features/index.ts +++ b/packages/@ember/canary-features/index.ts @@ -24,6 +24,7 @@ export const DEFAULT_FEATURES = { EMBER_NATIVE_DECORATOR_SUPPORT: true, EMBER_GLIMMER_FN_HELPER: null, EMBER_CUSTOM_COMPONENT_ARG_PROXY: null, + EMBER_GLIMMER_ON_MODIFIER: null, }; /** @@ -94,3 +95,4 @@ export const EMBER_GLIMMER_FN_HELPER = featureValue(FEATURES.EMBER_GLIMMER_FN_HE export const EMBER_CUSTOM_COMPONENT_ARG_PROXY = featureValue( FEATURES.EMBER_CUSTOM_COMPONENT_ARG_PROXY ); +export const EMBER_GLIMMER_ON_MODIFIER = featureValue(FEATURES.EMBER_GLIMMER_ON_MODIFIER); From 7e0cd03699befd84c53346e375d5af8e6689e92d Mon Sep 17 00:00:00 2001 From: Robert Jackson Date: Wed, 24 Apr 2019 10:41:49 -0400 Subject: [PATCH 2/2] Incorporate code review feedback from buschtoens. * Extract shared utility for a `this` context that errors when used. * Assert when more positional arguments are provided. * Avoid wrapping callback needlessly (makes debugging a bit easier, since the event handler function is actually the user-land function in most cases. * Incorporates a system to error if any `this` properties are accessed from within the callback in debug builds. * Fixup comments for addEventListener / removeEventListener logic. * Fix test assertion message. * Add test ensuring the modifier is destroyed properly. --- .../-internals/glimmer/lib/helpers/fn.ts | 36 +------------ .../-internals/glimmer/lib/modifiers/on.ts | 53 +++++++++++++------ .../glimmer/lib/utils/untouchable-this.ts | 39 ++++++++++++++ .../tests/integration/modifiers/on-test.js | 48 ++++++++++++++++- 4 files changed, 125 insertions(+), 51 deletions(-) create mode 100644 packages/@ember/-internals/glimmer/lib/utils/untouchable-this.ts diff --git a/packages/@ember/-internals/glimmer/lib/helpers/fn.ts b/packages/@ember/-internals/glimmer/lib/helpers/fn.ts index 9843f77604c..67accc55c31 100644 --- a/packages/@ember/-internals/glimmer/lib/helpers/fn.ts +++ b/packages/@ember/-internals/glimmer/lib/helpers/fn.ts @@ -1,43 +1,11 @@ -import { HAS_NATIVE_PROXY } from '@ember/-internals/utils'; import { assert } from '@ember/debug'; -import { DEBUG } from '@glimmer/env'; import { Arguments, VM } from '@glimmer/runtime'; import { ICapturedArguments } from '@glimmer/runtime/dist/types/lib/vm/arguments'; import { Opaque } from '@glimmer/util'; import { InternalHelperReference } from '../utils/references'; +import buildUntouchableThis from '../utils/untouchable-this'; -let context: any = null; -if (DEBUG && HAS_NATIVE_PROXY) { - let assertOnProperty = (property: string | number | symbol) => { - assert( - `You accessed \`this.${String( - property - )}\` from a function passed to the \`fn\` helper, but the function itself was not bound to a valid \`this\` context. Consider updating to usage of \`@action\`.` - ); - }; - - context = new Proxy( - {}, - { - get(_target: {}, property: string | symbol) { - assertOnProperty(property); - }, - - set(_target: {}, property: string | symbol) { - assertOnProperty(property); - - return false; - }, - - has(_target: {}, property: string | symbol) { - assertOnProperty(property); - - return false; - }, - } - ); -} - +const context = buildUntouchableThis('`fn` helper'); function fnHelper({ positional }: ICapturedArguments) { assert( `You must pass a function as the \`fn\` helpers first argument, you passed ${positional diff --git a/packages/@ember/-internals/glimmer/lib/modifiers/on.ts b/packages/@ember/-internals/glimmer/lib/modifiers/on.ts index 04841666fe7..54cbc4f2f1e 100644 --- a/packages/@ember/-internals/glimmer/lib/modifiers/on.ts +++ b/packages/@ember/-internals/glimmer/lib/modifiers/on.ts @@ -4,6 +4,9 @@ import { Opaque, Simple } from '@glimmer/interfaces'; import { Tag } from '@glimmer/reference'; import { Arguments, CapturedArguments, ModifierManager } from '@glimmer/runtime'; import { Destroyable } from '@glimmer/util'; +import buildUntouchableThis from '../utils/untouchable-this'; + +const untouchableContext = buildUntouchableThis('`on` modifier'); /* Internet Explorer 11 does not support `once` and also does not support @@ -105,21 +108,39 @@ export class OnModifierState { this.shouldUpdate = true; } + assert( + `You can only pass two positional arguments (event name and callback) to the \`on\` modifier, but you provided ${ + args.positional.length + }. Consider using the \`fn\` helper to provide additional arguments to the \`on\` callback.`, + args.positional.length === 2 + ); + + let needsCustomCallback = + (SUPPORTS_EVENT_OPTIONS === false && once) /* needs manual once implementation */ || + (DEBUG && passive) /* needs passive enforcement */; + if (this.shouldUpdate) { - let callback = (this.callback = function(this: Element, event) { - if (DEBUG && passive) { - event.preventDefault = () => { - assert( - `You marked this listener as 'passive', meaning that you must not call 'event.preventDefault()': \n\n${userProvidedCallback}` - ); - }; - } - - if (!SUPPORTS_EVENT_OPTIONS && once) { - removeEventListener(this, eventName, callback, options); - } - return userProvidedCallback.call(null, event); - }); + if (needsCustomCallback) { + let callback = (this.callback = function(this: Element, event) { + if (DEBUG && passive) { + event.preventDefault = () => { + assert( + `You marked this listener as 'passive', meaning that you must not call 'event.preventDefault()': \n\n${userProvidedCallback}` + ); + }; + } + + if (!SUPPORTS_EVENT_OPTIONS && once) { + removeEventListener(this, eventName, callback, options); + } + return userProvidedCallback.call(untouchableContext, event); + }); + } else if (DEBUG) { + // prevent the callback from being bound to the element + this.callback = userProvidedCallback.bind(untouchableContext); + } else { + this.callback = userProvidedCallback; + } } } @@ -156,7 +177,7 @@ function removeEventListener( // used only in the following cases: // // * where there is no options - // * `{ once: true | false, passive: true, capture: false } + // * `{ once: true | false, passive: true | false, capture: false } element.removeEventListener(eventName, callback); } } @@ -184,7 +205,7 @@ function addEventListener( // used only in the following cases: // // * where there is no options - // * `{ once: true | false, passive: true, capture: false } + // * `{ once: true | false, passive: true | false, capture: false } element.addEventListener(eventName, callback); } } diff --git a/packages/@ember/-internals/glimmer/lib/utils/untouchable-this.ts b/packages/@ember/-internals/glimmer/lib/utils/untouchable-this.ts new file mode 100644 index 00000000000..c58d61b5cc5 --- /dev/null +++ b/packages/@ember/-internals/glimmer/lib/utils/untouchable-this.ts @@ -0,0 +1,39 @@ +import { HAS_NATIVE_PROXY } from '@ember/-internals/utils'; +import { assert } from '@ember/debug'; +import { DEBUG } from '@glimmer/env'; + +export default function buildUntouchableThis(source: string): null | object { + let context: null | object = null; + if (DEBUG && HAS_NATIVE_PROXY) { + let assertOnProperty = (property: string | number | symbol) => { + assert( + `You accessed \`this.${String( + property + )}\` from a function passed to the ${source}, but the function itself was not bound to a valid \`this\` context. Consider updating to usage of \`@action\`.` + ); + }; + + context = new Proxy( + {}, + { + get(_target: {}, property: string | symbol) { + assertOnProperty(property); + }, + + set(_target: {}, property: string | symbol) { + assertOnProperty(property); + + return false; + }, + + has(_target: {}, property: string | symbol) { + assertOnProperty(property); + + return false; + }, + } + ); + } + + return context; +} diff --git a/packages/@ember/-internals/glimmer/tests/integration/modifiers/on-test.js b/packages/@ember/-internals/glimmer/tests/integration/modifiers/on-test.js index bdc79a6eaf8..df6d1f56a0b 100644 --- a/packages/@ember/-internals/glimmer/tests/integration/modifiers/on-test.js +++ b/packages/@ember/-internals/glimmer/tests/integration/modifiers/on-test.js @@ -43,7 +43,7 @@ if (EMBER_GLIMMER_ON_MODIFIER) { if (isChrome || isFirefox) { assert.strictEqual(SUPPORTS_EVENT_OPTIONS, true, 'is true in chrome and firefox'); } else if (isIE11) { - assert.strictEqual(SUPPORTS_EVENT_OPTIONS, false, 'is true in chrome and firefox'); + assert.strictEqual(SUPPORTS_EVENT_OPTIONS, false, 'is false in IE11'); } else { assert.expect(0); } @@ -319,6 +319,52 @@ if (EMBER_GLIMMER_ON_MODIFIER) { this.render(``, { foo: null }); }, /You must pass a function as the second argument to the `on` modifier/); } + + '@test asserts if the provided callback accesses `this` without being bound prior to passing to on'() { + this.render(``, { + myFunc() { + expectAssertion(() => { + this.arg1; + }, /You accessed `this.arg1` from a function passed to the `on` modifier, but the function itself was not bound to a valid `this` context. Consider updating to usage of `@action`./); + }, + + arg1: 'foo', + }); + + runTask(() => this.$('button').click()); + } + + '@test asserts if more than 2 positional parameters are provided'() { + expectAssertion(() => { + this.render(``, { + callback() {}, + someArg: 'foo', + }); + }, /You can only pass two positional arguments \(event name and callback\) to the `on` modifier, but you provided 3. Consider using the `fn` helper to provide additional arguments to the `on` callback./); + } + + '@test it removes the modifier when the element is removed'(assert) { + let count = 0; + + this.render( + '{{#if this.showButton}}{{/if}}', + { + callback() { + count++; + }, + showButton: true, + } + ); + + this.assertCounts({ adds: 1, removes: 0 }); + + runTask(() => this.$('button').click()); + assert.equal(count, 1, 'has been called 1 time'); + + runTask(() => this.context.set('showButton', false)); + + this.assertCounts({ adds: 1, removes: 1 }); + } } ); }