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.

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

[BUGFIX lts] Restore {{this.attrs.foo}} deprecation #20672

Merged
merged 1 commit into from
Mar 29, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -1270,10 +1270,13 @@ moduleFor(
}, "Using {{attrs}} to reference named arguments is not supported. {{attrs.someProp}} should be updated to {{@someProp}}. ('my-app/templates/components/non-block.hbs' @ L1:C24) ");
}

// Perhaps change this test to `{{this.attrs.someProp.value}}` when removing the deprecation?
['@test non-block with properties on this.attrs']() {
this.registerComponent('non-block', {
template: 'In layout - someProp: {{this.attrs.someProp}}',
});
expectDeprecation(() => {
this.registerComponent('non-block', {
template: 'In layout - someProp: {{this.attrs.someProp}}',
});
}, /Using {{this.attrs}} to reference named arguments has been deprecated. {{this.attrs.someProp}} should be updated to {{@someProp}}./);

this.render('{{non-block someProp=this.prop}}', {
prop: 'something here',
Expand Down Expand Up @@ -1477,21 +1480,24 @@ moduleFor(
);
}

// Perhaps change this test to `{{this.attrs.foo.value}}` when removing the deprecation?
['@test this.attrs.foo === @foo === foo']() {
this.registerComponent('foo-bar', {
template: strip`
Args: {{this.attrs.value}} | {{@value}} | {{this.value}}
{{#each this.attrs.items as |item|}}
{{item}}
{{/each}}
{{#each @items as |item|}}
{{item}}
{{/each}}
{{#each this.items as |item|}}
{{item}}
{{/each}}
`,
});
expectDeprecation(() => {
this.registerComponent('foo-bar', {
template: strip`
Args: {{this.attrs.value}} | {{@value}} | {{this.value}}
{{#each this.attrs.items as |item|}}
{{item}}
{{/each}}
{{#each @items as |item|}}
{{item}}
{{/each}}
{{#each this.items as |item|}}
{{item}}
{{/each}}
`,
});
}, /Using {{this.attrs}} to reference named arguments has been deprecated. {{this.attrs..+}} should be updated to {{@.+}}./);

this.render('{{foo-bar value=this.model.value items=this.model.items}}', {
model: {
Expand Down Expand Up @@ -1576,10 +1582,13 @@ moduleFor(
}, "Using {{attrs}} to reference named arguments is not supported. {{attrs.someProp}} should be updated to {{@someProp}}. ('my-app/templates/components/with-block.hbs' @ L1:C24) ");
}

// Perhaps change this test to `{{this.attrs.someProp.value}}` when removing the deprecation?
['@test block with properties on this.attrs']() {
this.registerComponent('with-block', {
template: 'In layout - someProp: {{this.attrs.someProp}} - {{yield}}',
});
expectDeprecation(() => {
this.registerComponent('with-block', {
template: 'In layout - someProp: {{this.attrs.someProp}} - {{yield}}',
});
}, /Using {{this.attrs}} to reference named arguments has been deprecated. {{this.attrs.someProp}} should be updated to {{@someProp}}./);

this.render(
strip`
Expand Down Expand Up @@ -3319,16 +3328,19 @@ moduleFor(
}, "Using {{attrs}} to reference named arguments is not supported. {{attrs.myVar}} should be updated to {{@myVar}}. ('my-app/templates/components/foo-bar.hbs' @ L1:C10) ");
}

// Perhaps change this test to `{{this.attrs.myVar.value}}` when removing the deprecation?
['@test using this.attrs for positional params']() {
let MyComponent = Component.extend();

this.registerComponent('foo-bar', {
ComponentClass: MyComponent.reopenClass({
positionalParams: ['myVar'],
}),
template:
'MyVar1: {{this.attrs.myVar}} {{this.myVar}} MyVar2: {{this.myVar2}} {{this.attrs.myVar2}}',
});
expectDeprecation(() => {
this.registerComponent('foo-bar', {
ComponentClass: MyComponent.reopenClass({
positionalParams: ['myVar'],
}),
template:
'MyVar1: {{this.attrs.myVar}} {{this.myVar}} MyVar2: {{this.myVar2}} {{this.attrs.myVar2}}',
});
}, /Using {{this.attrs}} to reference named arguments has been deprecated. {{this.attrs.myVar2?}} should be updated to {{@myVar2?}}./);

this.render('{{foo-bar 1 myVar2=2}}');

Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { assert } from '@ember/debug';
import { assert, deprecate } from '@ember/debug';
import type { AST, ASTPlugin } from '@glimmer/syntax';
import calculateLocationDisplay from '../system/calculate-location-display';
import type { EmberASTPluginEnvironment } from '../types';
Expand Down Expand Up @@ -56,17 +56,41 @@ export default function assertAgainstAttrs(env: EmberASTPluginEnvironment): ASTP

PathExpression(node: AST.PathExpression): AST.Node | void {
if (isAttrs(node, stack[stack.length - 1]!)) {
let path = b.path(node.original.substring(6));

assert(
`Using {{attrs}} to reference named arguments is not supported. {{attrs.${
path.original
}}} should be updated to {{@${path.original}}}. ${calculateLocationDisplay(
`Using {{attrs}} to reference named arguments is not supported. {{${
node.original
}}} should be updated to {{@${node.original.slice(6)}}}. ${calculateLocationDisplay(
moduleName,
node.loc
)}`
);
} else if (isThisDotAttrs(node)) {
// When removing this, ensure `{{this.attrs.foo}}` is left as-is, without triggering
// any assertions/deprecations. It's perfectly legal to reference `{{this.attrs.foo}}`
// in the template since it is a real property on the backing class – it will give you
// a `MutableCell` wrapper object, but maybe that's what you want. And in any case,
// there is no compelling to special case that property access.
deprecate(
`Using {{this.attrs}} to reference named arguments has been deprecated. {{${
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made a new thing somewhat recently -- you should use deprecateUntil from @ember/-internals/deprecations and also define the deprecation in there (there are docs in there).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated, it will make it slightly more work to back port, but it shouldn't be too bad.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to revert this – the build wasn't properly set up for ember-template-compiler to be able to import from @ember/-internals. I think it can probably be made to work, but it would require some refactoring and ensure that @ember/-internals doesn't accidentally depend on browser features since it'll also have to run in node.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given my other question #20669, I do wonder if we will be better off enforcing that at build time in wherever-that-babel-code-lives.

In any case, I think we can land this and come back to fix it later when/if that other problem gets resolved. It should also make it easier to back port in the meantime. I can add a manual assertion here for the ember-source version if you would like.

node.original
}}} should be updated to {{@${node.original.slice(11)}}}. ${calculateLocationDisplay(
moduleName,
node.loc
)}`,
node.this !== false
false,
{
id: 'attrs-arg-access',
url: 'https://deprecations.emberjs.com/v3.x/#toc_attrs-arg-access',
until: '6.0.0',
for: 'ember-source',
since: {
available: '3.26.0',
enabled: '3.26.0',
},
}
);

return b.path(`@${node.original.slice(11)}`, node.loc);
}
},
},
Expand All @@ -75,19 +99,10 @@ export default function assertAgainstAttrs(env: EmberASTPluginEnvironment): ASTP

function isAttrs(node: AST.PathExpression, symbols: string[]) {
let name = node.parts[0];
return node.head.type === 'VarHead' && name === 'attrs' && symbols.indexOf(name) === -1;
}

if (name && symbols.indexOf(name) !== -1) {
return false;
}

if (name === 'attrs') {
if (node.this === true) {
node.parts.shift();
node.original = node.original.slice(5);
}

return true;
}

return false;
function isThisDotAttrs(node: AST.PathExpression) {
let name = node.parts[0];
return node.head.type === 'ThisHead' && name === 'attrs';
}
Original file line number Diff line number Diff line change
Expand Up @@ -29,14 +29,26 @@ moduleFor(
}, /Using {{attrs}} to reference named arguments is not supported. {{attrs.foo.bar}} should be updated to {{@foo.bar}}./);
}

['@test it does not assert against this.attrs']() {
this.assertTransformed(`{{this.attrs.foo}}`, `{{this.attrs.foo}}`);
this.assertTransformed(`{{if this.attrs.foo "foo"}}`, `{{if this.attrs.foo "foo"}}`);
this.assertTransformed(`{{#if this.attrs.foo}}{{/if}}`, `{{#if this.attrs.foo}}{{/if}}`);
this.assertTransformed(
`{{deeply (nested this.attrs.foo.bar)}}`,
`{{deeply (nested this.attrs.foo.bar)}}`
);
// When removing the deprecation, ensure `{{this.attrs.foo}}` isn't rewritten and does NOT trigger any assertions/deprecations
['@test this.attrs is deprecated']() {
expectDeprecation(() => {
this.assertTransformed(`{{this.attrs.foo}}`, `{{@foo}}`);
}, /Using {{this.attrs}} to reference named arguments has been deprecated. {{this.attrs.foo}} should be updated to {{@foo}}./);

expectDeprecation(() => {
this.assertTransformed(`{{if this.attrs.foo "foo"}}`, `{{if @foo "foo"}}`);
}, /Using {{this.attrs}} to reference named arguments has been deprecated. {{this.attrs.foo}} should be updated to {{@foo}}./);

expectDeprecation(() => {
this.assertTransformed(`{{#if this.attrs.foo}}{{/if}}`, `{{#if @foo}}{{/if}}`);
}, /Using {{this.attrs}} to reference named arguments has been deprecated. {{this.attrs.foo}} should be updated to {{@foo}}./);

expectDeprecation(() => {
this.assertTransformed(
`{{deeply (nested this.attrs.foo.bar)}}`,
`{{deeply (nested @foo.bar)}}`
);
}, /Using {{this.attrs}} to reference named arguments has been deprecated. {{this.attrs.foo.bar}} should be updated to {{@foo.bar}}./);
}
}
);
Expand Down