Skip to content

Commit

Permalink
Remove incorrect assertions
Browse files Browse the repository at this point in the history 
If you use `render` on a component and then call `this.set`, you get an assertion:

>'You cannot call `this.set` when passing a component to `render()` (the rendered component does not have access to the test context).',

But that's not true and hasn't been since the introduction of components with access to lexical scope. For example:

```gjs
let self = this;
render(<template>{{self.message}}</template>)
this.set('message', 'updated')
```

My example uses a workaround for emberjs/babel-plugin-ember-template-compilation#61, but we have open PRs fixing that issue in which case this would also just work directly:

```gjs
render(<template>{{this.message}}</template>)
this.set('message', 'updated')
```

Critically, `@embroider/template-tag-codemod` can produce these situations entirely automatically, as it upgrades any existing tests to template tag.
  • Loading branch information
@ef4
ef4 committed Jan 16, 2025
1 parent 0505fdf commit 419b1fa
Show file tree
Hide file tree
Showing 3 changed files with 0 additions and 196 deletions.
41 changes: 0 additions & 41 deletions addon/src/setup-context.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -355,11 +355,6 @@ 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<BaseContext, true>();
export const SetUsage = new WeakMap<BaseContext, Array<string>>();

/**
Used by test framework addons to setup the provided context for testing.
Expand Down Expand Up @@ -432,24 +427,8 @@ export default function setupContext<T extends object>(
// SAFETY: in all of these `defineProperty` calls, we can't actually guarantee any safety w.r.t. the corresponding field's type in `TestContext`
value(key: any, value: any): unknown {
const ret = run(function () {
if (ComponentRenderMap.has(context)) {
assert(
'You cannot call `this.set` when passing a component to `render()` (the rendered component does not have access to the test context).',
);
} else {
let setCalls = SetUsage.get(context);

if (setCalls === undefined) {
setCalls = [];
SetUsage.set(context, setCalls);
}

setCalls?.push(key);
}

return set(context, key, value);
});

return ret;
},
writable: false,
Expand All @@ -461,26 +440,6 @@ export default function setupContext<T extends object>(
// SAFETY: in all of these `defineProperty` calls, we can't actually guarantee any safety w.r.t. the corresponding field's type in `TestContext`
value(hash: any): unknown {
const ret = run(function () {
if (ComponentRenderMap.has(context)) {
assert(
'You cannot call `this.setProperties` when passing a component to `render()` (the rendered component does not have access to the test context)',
);
} else {
// While neither the types nor the API documentation indicate that passing `null` or
// `undefined` to `setProperties` is allowed, it works and *has worked* for a long
// time, so there is considerable real-world code which relies on the fact that it
// does in fact work. Checking and no-op-ing here handles that.
if (hash != null) {
let setCalls = SetUsage.get(context);

if (SetUsage.get(context) === undefined) {
setCalls = [];
SetUsage.set(context, setCalls);
}

setCalls?.push(...Object.keys(hash));
}
}
return setProperties(context, hash);
});

Expand Down
15 changes: 0 additions & 15 deletions addon/src/setup-rendering-context.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import { assert } from '@ember/debug';
import { runHooks } from './helper-hooks.ts';
import hasEmberVersion from './has-ember-version.ts';
import isComponent from './-internal/is-component.ts';
import { ComponentRenderMap, SetUsage } from './setup-context.ts';

// the built in types do not provide types for @ember/template-compilation
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
Expand Down Expand Up @@ -134,20 +133,6 @@ export function render(
const ownerToRenderFrom = options?.owner || owner;

if (isComponent(templateFactoryOrComponent)) {
// 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);

const setCalls = SetUsage.get(context);

if (setCalls !== undefined) {
assert(
`You cannot call \`this.set\` or \`this.setProperties\` when passing a component to \`render\`, but they were called for the following properties:\n${setCalls
.map((key) => ` - ${key}`)
.join('\n')}`,
);
}

context = {
ProvidedComponent: templateFactoryOrComponent,
};
Expand Down
140 changes: 0 additions & 140 deletions test-app/tests/unit/setup-rendering-context-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -654,76 +654,6 @@ module('setupRenderingContext', function (hooks) {
assert.equal(this.element.textContent, 'the value of foo is foo');
});

test('setting properties on the test context *before* rendering a component throws an assertion', async function (assert) {
assert.expect(1);
const template = precompileTemplate(
'the value of foo is {{this.foo}}'
);

class Foo extends GlimmerComponent {}

const component = setComponentTemplate(template, Foo);

this.set('foo', 'FOO');
this.set('bar', 'BAR');
this.setProperties({
baz: 'BAZ',
baq: 'BAQ',
});

let error;
try {
await render(component);
} catch (err) {
error = err;
} finally {
assert.equal(
error.toString(),
`Error: Assertion Failed: You cannot call \`this.set\` or \`this.setProperties\` when passing a component to \`render\`, but they were called for the following properties:
- foo
- bar
- baz
- baq`
);
}
});

test('setting properties on the test context *after* rendering a component throws an assertion', async function (assert) {
const template = precompileTemplate(
'the value of foo is {{this.foo}}'
);

class Foo extends GlimmerComponent {}

const component = setComponentTemplate(template, Foo);

await render(component);

assert.throws(
() => this.set('foo', 'FOO'),
(err) => {
return err
.toString()
.includes(
'You cannot call `this.set` when passing a component to `render()` (the rendered component does not have access to the test context).'
);
},
'errors on this.set'
);

assert.throws(
() => this.setProperties({ foo: 'bar?' }),
(err) => {
return err
.toString()
.includes(
'You cannot call `this.setProperties` when passing a component to `render()` (the rendered component does not have access to the test context)'
);
},
'errors on this.setProperties'
);
});

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 Down Expand Up @@ -811,76 +741,6 @@ module('setupRenderingContext', function (hooks) {
assert.equal(this.element.textContent, 'the value of foo is foo');
});

test('setting properties on the test context *before* rendering a component throws an assertion', async function (assert) {
assert.expect(1);
const template = precompileTemplate(
'the value of foo is {{this.foo}}'
);

class Foo extends GlimmerComponent {}

const component = setComponentTemplate(template, Foo);

this.set('foo', 'FOO');
this.set('bar', 'BAR');
this.setProperties({
baz: 'BAZ',
baq: 'BAQ',
});

let error;
try {
await render(component);
} catch (err) {
error = err;
} finally {
assert.equal(
error.toString(),
`Error: Assertion Failed: You cannot call \`this.set\` or \`this.setProperties\` when passing a component to \`render\`, but they were called for the following properties:
- foo
- bar
- baz
- baq`
);
}
});

test('setting properties on the test context *after* rendering a component throws an assertion', async function (assert) {
const template = precompileTemplate(
'the value of foo is {{this.foo}}'
);

class Foo extends GlimmerComponent {}

const component = setComponentTemplate(template, Foo);

await render(component);

assert.throws(
() => this.set('foo', 'FOO'),
(err) => {
return err
.toString()
.includes(
'You cannot call `this.set` when passing a component to `render()` (the rendered component does not have access to the test context).'
);
},
'errors on this.set'
);

assert.throws(
() => this.setProperties({ foo: 'bar?' }),
(err) => {
return err
.toString()
.includes(
'You cannot call `this.setProperties` when passing a component to `render()` (the rendered component does not have access to the test context)'
);
},
'errors on this.setProperties'
);
});

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 Down

0 comments on commit 419b1fa

Please sign in to comment.