Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove incorrect assertions #1527

Merged
merged 1 commit into from
Jan 17, 2025
+0 −197
Merged

Remove incorrect assertions #1527

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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.
@ef4
ef4 committed Jan 16, 2025
commit 7e0340f2f5acf70aadcad44a48d9d82be70250fe
41 changes: 0 additions & 41 deletions addon/src/setup-context.ts
View file
Original file line number Diff line number Diff line change
@@ -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.

@@ -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,
@@ -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);
});

16 changes: 0 additions & 16 deletions addon/src/setup-rendering-context.ts
View file
Original file line number Diff line number Diff line change
@@ -11,11 +11,9 @@ import settled from './settled.ts';
import getRootElement from './dom/get-root-element.ts';
import type { Owner } from './build-owner.ts';
import getTestMetadata from './test-metadata.ts';
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 +132,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,
};
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
@@ -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}}'