From 419b1fa0bee49667b42f7b362e88eadef2bc0729 Mon Sep 17 00:00:00 2001 From: Edward Faulkner Date: Thu, 16 Jan 2025 17:30:18 -0500 Subject: [PATCH] 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() this.set('message', 'updated') ``` My example uses a workaround for https://github.com/emberjs/babel-plugin-ember-template-compilation/issues/61, but we have open PRs fixing that issue in which case this would also just work directly: ```gjs render() this.set('message', 'updated') ``` Critically, `@embroider/template-tag-codemod` can produce these situations entirely automatically, as it upgrades any existing tests to template tag. --- addon/src/setup-context.ts | 41 ----- addon/src/setup-rendering-context.ts | 15 -- .../unit/setup-rendering-context-test.js | 140 ------------------ 3 files changed, 196 deletions(-) diff --git a/addon/src/setup-context.ts b/addon/src/setup-context.ts index 6a434ba89..e373de09f 100644 --- a/addon/src/setup-context.ts +++ b/addon/src/setup-context.ts @@ -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(); -export const SetUsage = new WeakMap>(); - /** Used by test framework addons to setup the provided context for testing. @@ -432,24 +427,8 @@ export default function setupContext( // 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, @@ -461,26 +440,6 @@ export default function setupContext( // 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); }); diff --git a/addon/src/setup-rendering-context.ts b/addon/src/setup-rendering-context.ts index d2581d75d..4900e56fd 100644 --- a/addon/src/setup-rendering-context.ts +++ b/addon/src/setup-rendering-context.ts @@ -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 @@ -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, }; diff --git a/test-app/tests/unit/setup-rendering-context-test.js b/test-app/tests/unit/setup-rendering-context-test.js index cd16fb953..0d2bd103d 100644 --- a/test-app/tests/unit/setup-rendering-context-test.js +++ b/test-app/tests/unit/setup-rendering-context-test.js @@ -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}}' @@ -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}}'