Skip to content

Commit 419b1fa

Browse files
ef4ef4
committed
Remove incorrect assertions
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.
1 parent 0505fdf commit 419b1fa

File tree

3 files changed

+0
-196
lines changed

3 files changed

+0
-196
lines changed

‎addon/src/setup-context.ts

-41
Original file line numberDiff line numberDiff line change
@@ -355,11 +355,6 @@ export function getWarningsDuringCallback(
355355
return getWarningsDuringCallbackForContext(context, callback);
356356
}
357357

358-
// This WeakMap is used to track whenever a component is rendered in a test so that we can throw
359-
// assertions when someone uses `this.{set,setProperties}` while rendering a component.
360-
export const ComponentRenderMap = new WeakMap<BaseContext, true>();
361-
export const SetUsage = new WeakMap<BaseContext, Array<string>>();
362-
363358
/**
364359
Used by test framework addons to setup the provided context for testing.
365360
@@ -432,24 +427,8 @@ export default function setupContext<T extends object>(
432427
// SAFETY: in all of these `defineProperty` calls, we can't actually guarantee any safety w.r.t. the corresponding field's type in `TestContext`
433428
value(key: any, value: any): unknown {
434429
const ret = run(function () {
435-
if (ComponentRenderMap.has(context)) {
436-
assert(
437-
'You cannot call `this.set` when passing a component to `render()` (the rendered component does not have access to the test context).',
438-
);
439-
} else {
440-
let setCalls = SetUsage.get(context);
441-
442-
if (setCalls === undefined) {
443-
setCalls = [];
444-
SetUsage.set(context, setCalls);
445-
}
446-
447-
setCalls?.push(key);
448-
}
449-
450430
return set(context, key, value);
451431
});
452-
453432
return ret;
454433
},
455434
writable: false,
@@ -461,26 +440,6 @@ export default function setupContext<T extends object>(
461440
// SAFETY: in all of these `defineProperty` calls, we can't actually guarantee any safety w.r.t. the corresponding field's type in `TestContext`
462441
value(hash: any): unknown {
463442
const ret = run(function () {
464-
if (ComponentRenderMap.has(context)) {
465-
assert(
466-
'You cannot call `this.setProperties` when passing a component to `render()` (the rendered component does not have access to the test context)',
467-
);
468-
} else {
469-
// While neither the types nor the API documentation indicate that passing `null` or
470-
// `undefined` to `setProperties` is allowed, it works and *has worked* for a long
471-
// time, so there is considerable real-world code which relies on the fact that it
472-
// does in fact work. Checking and no-op-ing here handles that.
473-
if (hash != null) {
474-
let setCalls = SetUsage.get(context);
475-
476-
if (SetUsage.get(context) === undefined) {
477-
setCalls = [];
478-
SetUsage.set(context, setCalls);
479-
}
480-
481-
setCalls?.push(...Object.keys(hash));
482-
}
483-
}
484443
return setProperties(context, hash);
485444
});
486445

‎addon/src/setup-rendering-context.ts

-15
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@ import { assert } from '@ember/debug';
1515
import { runHooks } from './helper-hooks.ts';
1616
import hasEmberVersion from './has-ember-version.ts';
1717
import isComponent from './-internal/is-component.ts';
18-
import { ComponentRenderMap, SetUsage } from './setup-context.ts';
1918

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

136135
if (isComponent(templateFactoryOrComponent)) {
137-
// We use this to track when `render` is used with a component so that we can throw an
138-
// assertion if `this.{set,setProperty} is used in the same test
139-
ComponentRenderMap.set(context, true);
140-
141-
const setCalls = SetUsage.get(context);
142-
143-
if (setCalls !== undefined) {
144-
assert(
145-
`You cannot call \`this.set\` or \`this.setProperties\` when passing a component to \`render\`, but they were called for the following properties:\n${setCalls
146-
.map((key) => ` - ${key}`)
147-
.join('\n')}`,
148-
);
149-
}
150-
151136
context = {
152137
ProvidedComponent: templateFactoryOrComponent,
153138
};

‎test-app/tests/unit/setup-rendering-context-test.js

-140
Original file line numberDiff line numberDiff line change
@@ -654,76 +654,6 @@ module('setupRenderingContext', function (hooks) {
654654
assert.equal(this.element.textContent, 'the value of foo is foo');
655655
});
656656

657-
test('setting properties on the test context *before* rendering a component throws an assertion', async function (assert) {
658-
assert.expect(1);
659-
const template = precompileTemplate(
660-
'the value of foo is {{this.foo}}'
661-
);
662-
663-
class Foo extends GlimmerComponent {}
664-
665-
const component = setComponentTemplate(template, Foo);
666-
667-
this.set('foo', 'FOO');
668-
this.set('bar', 'BAR');
669-
this.setProperties({
670-
baz: 'BAZ',
671-
baq: 'BAQ',
672-
});
673-
674-
let error;
675-
try {
676-
await render(component);
677-
} catch (err) {
678-
error = err;
679-
} finally {
680-
assert.equal(
681-
error.toString(),
682-
`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:
683-
- foo
684-
- bar
685-
- baz
686-
- baq`
687-
);
688-
}
689-
});
690-
691-
test('setting properties on the test context *after* rendering a component throws an assertion', async function (assert) {
692-
const template = precompileTemplate(
693-
'the value of foo is {{this.foo}}'
694-
);
695-
696-
class Foo extends GlimmerComponent {}
697-
698-
const component = setComponentTemplate(template, Foo);
699-
700-
await render(component);
701-
702-
assert.throws(
703-
() => this.set('foo', 'FOO'),
704-
(err) => {
705-
return err
706-
.toString()
707-
.includes(
708-
'You cannot call `this.set` when passing a component to `render()` (the rendered component does not have access to the test context).'
709-
);
710-
},
711-
'errors on this.set'
712-
);
713-
714-
assert.throws(
715-
() => this.setProperties({ foo: 'bar?' }),
716-
(err) => {
717-
return err
718-
.toString()
719-
.includes(
720-
'You cannot call `this.setProperties` when passing a component to `render()` (the rendered component does not have access to the test context)'
721-
);
722-
},
723-
'errors on this.setProperties'
724-
);
725-
});
726-
727657
test('setting properties on the test context when rendering a template does not throw an assertion', async function (assert) {
728658
const template = precompileTemplate(
729659
'the value of foo is {{this.foo}}'
@@ -811,76 +741,6 @@ module('setupRenderingContext', function (hooks) {
811741
assert.equal(this.element.textContent, 'the value of foo is foo');
812742
});
813743

814-
test('setting properties on the test context *before* rendering a component throws an assertion', async function (assert) {
815-
assert.expect(1);
816-
const template = precompileTemplate(
817-
'the value of foo is {{this.foo}}'
818-
);
819-
820-
class Foo extends GlimmerComponent {}
821-
822-
const component = setComponentTemplate(template, Foo);
823-
824-
this.set('foo', 'FOO');
825-
this.set('bar', 'BAR');
826-
this.setProperties({
827-
baz: 'BAZ',
828-
baq: 'BAQ',
829-
});
830-
831-
let error;
832-
try {
833-
await render(component);
834-
} catch (err) {
835-
error = err;
836-
} finally {
837-
assert.equal(
838-
error.toString(),
839-
`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:
840-
- foo
841-
- bar
842-
- baz
843-
- baq`
844-
);
845-
}
846-
});
847-
848-
test('setting properties on the test context *after* rendering a component throws an assertion', async function (assert) {
849-
const template = precompileTemplate(
850-
'the value of foo is {{this.foo}}'
851-
);
852-
853-
class Foo extends GlimmerComponent {}
854-
855-
const component = setComponentTemplate(template, Foo);
856-
857-
await render(component);
858-
859-
assert.throws(
860-
() => this.set('foo', 'FOO'),
861-
(err) => {
862-
return err
863-
.toString()
864-
.includes(
865-
'You cannot call `this.set` when passing a component to `render()` (the rendered component does not have access to the test context).'
866-
);
867-
},
868-
'errors on this.set'
869-
);
870-
871-
assert.throws(
872-
() => this.setProperties({ foo: 'bar?' }),
873-
(err) => {
874-
return err
875-
.toString()
876-
.includes(
877-
'You cannot call `this.setProperties` when passing a component to `render()` (the rendered component does not have access to the test context)'
878-
);
879-
},
880-
'errors on this.setProperties'
881-
);
882-
});
883-
884744
test('setting properties on the test context when rendering a template does not throw an assertion', async function (assert) {
885745
const template = precompileTemplate(
886746
'the value of foo is {{this.foo}}'

0 commit comments

Comments
 (0)