Skip to content

Commit

Permalink
throw when using this.set with a component
Browse files Browse the repository at this point in the history
  • Loading branch information
@cafreeman
cafreeman committed Apr 12, 2022
1 parent 8d935fb commit a749f81
Show file tree
Hide file tree
Showing 3 changed files with 70 additions and 91 deletions.
19 changes: 14 additions & 5 deletions addon-test-support/@ember/test-helpers/setup-context.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,6 @@ import {
Warning,
} from './-internal/warnings';

export const CALLED_SET = '__INTERNAL__called_set';
export const CALLED_SET_PROPERTIES = '__INTERNAL__called_set_properties';

// This handler exists to provide the underlying data to enable the following methods:
// * getDeprecations()
// * getDeprecationsDuringCallback()
Expand Down Expand Up @@ -345,6 +342,10 @@ export function getWarningsDuringCallback(
return getWarningsDuringCallbackForContext(context, callback);
}

// This WeakMap is used to track whenever a component is rendered in a test so that we can throw
// assertions when someone uses `this.{set,setProperties}` while rendering a component.
export const ComponentRenderMap = new WeakMap();

/**
Used by test framework addons to setup the provided context for testing.
Expand Down Expand Up @@ -417,7 +418,11 @@ export default function setupContext(
enumerable: true,
value(key: string, value: any): any {
let ret = run(function () {
set(context, CALLED_SET, true);
if (ComponentRenderMap.has(context)) {
assert(
'Calling `this.set` when rendering a component does not work since components do not have access to the test context.'
);
}
return set(context, key, value);
});

Expand All @@ -431,7 +436,11 @@ export default function setupContext(
enumerable: true,
value(hash: { [key: string]: any }): { [key: string]: any } {
let ret = run(function () {
set(context, CALLED_SET_PROPERTIES, true);
if (ComponentRenderMap.has(context)) {
assert(
'Calling `this.setProperties` when rendering a component does not work since components do not have access to the test context.'
);
}
return setProperties(context, hash);
});

Expand Down
21 changes: 4 additions & 17 deletions addon-test-support/@ember/test-helpers/setup-rendering-context.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,7 @@ import { runHooks } from './-internal/helper-hooks';
import hasEmberVersion from './has-ember-version';
import isComponent from './-internal/is-component';
import { macroCondition, dependencySatisfies } from '@embroider/macros';
import { warn } from '@ember/debug';
import { CALLED_SET, CALLED_SET_PROPERTIES } from './setup-context';
import { ComponentRenderMap } from './setup-context';
import type { ComponentInstance } from '@glimmer/interfaces';

const OUTLET_TEMPLATE = hbs`{{outlet}}`;
Expand Down Expand Up @@ -144,21 +143,9 @@ export function render(
);
} else {
if (isComponent(templateOrComponent, owner)) {
if (context.get(CALLED_SET)) {
warn(
"Calling `this.set` when rendering a component is an epic troll. Probably don't do it.",
{ id: 'set-warning' }
);
}

if (context.get(CALLED_SET_PROPERTIES)) {
warn(
"Calling `this.setProperties` when rendering a component is an epic troll. Probably don't do it.",
{
id: 'set-properties-warning',
}
);
}
// We use this to track when `render` is used with a component so that we can throw an
// assertion if `this.{set,setProperty} is used in the same test
ComponentRenderMap.set(context, true);

if (
macroCondition(
Expand Down
121 changes: 52 additions & 69 deletions tests/unit/setup-rendering-context-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ import {
getTestMetadata,
render,
rerender,
getWarnings,
clearRender,
setApplication,
setResolver,
Expand Down Expand Up @@ -650,16 +649,15 @@ module('setupRenderingContext', function (hooks) {
}

const component = setComponentTemplate(template, Foo);
await render(component);

assert.equal(this.element.textContent, 'the value of foo is foo');

this.set('foo', 'bar');
await rerender();

await render(component);

assert.equal(this.element.textContent, 'the value of foo is foo');
});

test('setting properties on the test context when rendering a component triggers a warning', async function (assert) {
test('setting properties on the test context when rendering a component throws an assertion', async function (assert) {
const template = precompileTemplate(
'the value of foo is {{this.foo}}'
);
Expand All @@ -668,29 +666,34 @@ module('setupRenderingContext', function (hooks) {

const component = setComponentTemplate(template, Foo);

this.set('foo', 'FOO');
this.setProperties({
foo: 'bar?',
});

await render(component);

const warnings = getWarnings();

assert.ok(
warnings.some((v) => v.options && v.options.id === 'set-warning'),
'throws a warning for this.set'
assert.throws(
() => this.set('foo', 'FOO'),
(err) => {
return err
.toString()
.includes(
'Calling `this.set` when rendering a component does not work since components do not have access to the test context.'
);
},
'errors on this.set'
);

assert.ok(
warnings.some(
(v) => v.options && v.options.id === 'set-properties-warning'
),
'throws a warning for this.setProperties'
assert.throws(
() => this.setProperties({ foo: 'bar?' }),
(err) => {
return err
.toString()
.includes(
'Calling `this.setProperties` when rendering a component does not work since components do not have access to the test context.'
);
},
'errors on this.setProperties'
);
});

test('setting properties on the test context when rendering a template does not trigger a warning', async function (assert) {
test('setting properties on the test context when rendering a template does not throw an assertion', async function (assert) {
const template = precompileTemplate(
'the value of foo is {{this.foo}}'
);
Expand All @@ -702,19 +705,7 @@ module('setupRenderingContext', function (hooks) {

await render(template);

const warnings = getWarnings();

assert.notOk(
warnings.some((v) => v.options && v.options.id === 'set-warning'),
'does not throw a warning for this.set'
);

assert.notOk(
warnings.some(
(v) => v.options && v.options.id === 'set-properties-warning'
),
'does not throw a warning for this.setProperties'
);
assert.true(true, 'no assertions are thrown');
});
});
} else if (hasEmberVersion(3, 24)) {
Expand Down Expand Up @@ -781,16 +772,15 @@ module('setupRenderingContext', function (hooks) {
}

const component = setComponentTemplate(template, Foo);
await render(component);

assert.equal(this.element.textContent, 'the value of foo is foo');

this.set('foo', 'bar');
await rerender();

await render(component);

assert.equal(this.element.textContent, 'the value of foo is foo');
});

test('setting properties on the test context when rendering a component triggers a warning', async function (assert) {
test('setting properties on the test context when rendering a component throws an assertion', async function (assert) {
const template = precompileTemplate(
'the value of foo is {{this.foo}}'
);
Expand All @@ -799,29 +789,34 @@ module('setupRenderingContext', function (hooks) {

const component = setComponentTemplate(template, Foo);

this.set('foo', 'FOO');
this.setProperties({
foo: 'bar?',
});

await render(component);

const warnings = getWarnings();

assert.ok(
warnings.some((v) => v.options && v.options.id === 'set-warning'),
'throws a warning for this.set'
assert.throws(
() => this.set('foo', 'FOO'),
(err) => {
return err
.toString()
.includes(
'Calling `this.set` when rendering a component does not work since components do not have access to the test context.'
);
},
'errors on this.set'
);

assert.ok(
warnings.some(
(v) => v.options && v.options.id === 'set-properties-warning'
),
'throws a warning for this.setProperties'
assert.throws(
() => this.setProperties({ foo: 'bar?' }),
(err) => {
return err
.toString()
.includes(
'Calling `this.setProperties` when rendering a component does not work since components do not have access to the test context.'
);
},
'errors on this.setProperties'
);
});

test('setting properties on the test context when rendering a template does not trigger a warning', async function (assert) {
test('setting properties on the test context when rendering a template does not throw an assertion', async function (assert) {
const template = precompileTemplate(
'the value of foo is {{this.foo}}'
);
Expand All @@ -833,19 +828,7 @@ module('setupRenderingContext', function (hooks) {

await render(template);

const warnings = getWarnings();

assert.notOk(
warnings.some((v) => v.options && v.options.id === 'set-warning'),
'does not throw a warning for this.set'
);

assert.notOk(
warnings.some(
(v) => v.options && v.options.id === 'set-properties-warning'
),
'does not throw a warning for this.setProperties'
);
assert.true(true, 'no assertions are thrown');
});
});
}
Expand Down

0 comments on commit a749f81

Please sign in to comment.