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 beta] internal templates should be strictMode #20603

Merged
merged 1 commit into from
Dec 12, 2023
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
1 change: 1 addition & 0 deletions packages/@ember/-internals/glimmer/lib/templates/empty.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { precompileTemplate } from '@ember/template-compilation';
export default precompileTemplate('', {
moduleName: 'packages/@ember/-internals/glimmer/lib/templates/empty.hbs',
strictMode: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one was not a problem, I just updated all of them to strict mode because why not?

});
9 changes: 8 additions & 1 deletion packages/@ember/-internals/glimmer/lib/templates/input.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { precompileTemplate } from '@ember/template-compilation';
import { on } from '@ember/modifier';
export default precompileTemplate(
`<input
{{!-- for compatibility --}}
Expand All @@ -17,5 +18,11 @@ export default precompileTemplate(
{{on "paste" this.valueDidChange}}
{{on "cut" this.valueDidChange}}
/>`,
{ moduleName: 'packages/@ember/-internals/glimmer/lib/templates/input.hbs' }
{
moduleName: 'packages/@ember/-internals/glimmer/lib/templates/input.hbs',
strictMode: true,
scope() {
return { on };
},
}
);
10 changes: 9 additions & 1 deletion packages/@ember/-internals/glimmer/lib/templates/link-to.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
import { precompileTemplate } from '@ember/template-compilation';
import { on } from '@ember/modifier';

export default precompileTemplate(
`<a
{{!-- for compatibility --}}
Expand All @@ -18,5 +20,11 @@ export default precompileTemplate(

{{on 'click' this.click}}
>{{yield}}</a>`,
{ moduleName: 'packages/@ember/-internals/glimmer/lib/templates/link-to.hbs' }
{
moduleName: 'packages/@ember/-internals/glimmer/lib/templates/link-to.hbs',
strictMode: true,
scope() {
return { on };
},
}
);
8 changes: 7 additions & 1 deletion packages/@ember/-internals/glimmer/lib/templates/outlet.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,10 @@
import { precompileTemplate } from '@ember/template-compilation';
export default precompileTemplate(`{{component (-outlet)}}`, {
import { outletHelper } from '../syntax/outlet';

export default precompileTemplate(`{{component (outletHelper)}}`, {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was one of the problematic cases. In loose mode, this could resolve a string into a component, which would be unsafe to optimize. In strict mode that kind of resolution is forbidden, so embroider can tell this is safe.

moduleName: 'packages/@ember/-internals/glimmer/lib/templates/outlet.hbs',
strictMode: true,
scope() {
return { outletHelper };
},
});
1 change: 1 addition & 0 deletions packages/@ember/-internals/glimmer/lib/templates/root.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { precompileTemplate } from '@ember/template-compilation';
export default precompileTemplate(`{{component this}}`, {
moduleName: 'packages/@ember/-internals/glimmer/lib/templates/root.hbs',
strictMode: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Notice that this benefits from being strictMode and does not need anything from scope. Hence the type declaration change in this PR.

});
10 changes: 9 additions & 1 deletion packages/@ember/-internals/glimmer/lib/templates/textarea.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
import { precompileTemplate } from '@ember/template-compilation';
import { on } from '@ember/modifier';

export default precompileTemplate(
`<textarea
{{!-- for compatibility --}}
Expand All @@ -15,5 +17,11 @@ export default precompileTemplate(
{{on "paste" this.valueDidChange}}
{{on "cut" this.valueDidChange}}
/>`,
{ moduleName: 'packages/@ember/-internals/glimmer/lib/templates/textarea.hbs' }
{
moduleName: 'packages/@ember/-internals/glimmer/lib/templates/textarea.hbs',
strictMode: true,
scope() {
return { on };
},
}
);
1 change: 1 addition & 0 deletions packages/@ember/-internals/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
"@ember/enumerable": "workspace:*",
"@ember/helper": "workspace:*",
"@ember/instrumentation": "workspace:*",
"@ember/modifier": "workspace:*",
"@ember/object": "workspace:*",
"@ember/owner": "workspace:*",
"@ember/routing": "workspace:*",
Expand Down
23 changes: 8 additions & 15 deletions packages/@ember/template-compilation/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,28 +2,21 @@ import { DEBUG } from '@glimmer/env';
import type { TemplateFactory } from '@glimmer/interfaces';
import type * as ETC from 'ember-template-compiler';

interface CommonOptions {
moduleName?: string;
}

interface LooseModeOptions extends CommonOptions {
strictMode?: false;
}

interface StrictModeOptions extends CommonOptions {
strictMode: true;
scope: () => Record<string, unknown>;
}

// (UN)SAFETY: the public API is that people can import and use this (and indeed
// it is emitted as part of Ember's build!), so we define it as having the type
// which makes that work. However, in practice it is supplied by the build,
// *for* the build, and will *not* be present at runtime, so the actual value
// here is `undefined` in prod; in dev it is a function which throws a somewhat
// nicer error. This is janky, but... here we are.
interface PrecompileTemplate {
(templateString: string, options?: LooseModeOptions): TemplateFactory;
(templateString: string, options: StrictModeOptions): TemplateFactory;
(
templateString: string,
options?: {
strictMode?: boolean;
scope?: () => Record<string, unknown>;
moduleName?: string;
}
): TemplateFactory;
}

export let __emberTemplateCompiler: undefined | typeof ETC;
Expand Down
2 changes: 1 addition & 1 deletion packages/@ember/test/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ registerWaiter = testingNotAvailableMessage;
unregisterHelper = testingNotAvailableMessage;
unregisterWaiter = testingNotAvailableMessage;

export function registerTestImplementaiton(impl: typeof EmberTesting) {
export function registerTestImplementation(impl: typeof EmberTesting) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is newly-added private API appearing for the first time in this beta. Seems a good time to fix my typo.

let { Test } = impl;
registerAsyncHelper = Test.registerAsyncHelper;
registerHelper = Test.registerHelper;
Expand Down
4 changes: 2 additions & 2 deletions packages/ember-testing/index.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
export * from './lib/public-api';
import * as EmberTesting from './lib/public-api';
import { registerTestImplementaiton } from '@ember/test';
import { registerTestImplementation } from '@ember/test';

registerTestImplementaiton(EmberTesting);
registerTestImplementation(EmberTesting);
5 changes: 4 additions & 1 deletion pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 0 additions & 6 deletions type-tests/@ember/template-compilation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,6 @@ precompileTemplate(`Hello World`, { strictMode: true, moduleName: 'hello', scope
// Integration, since this is the primary use case for precompileTemplate
expectTypeOf(setComponentTemplate(precompileTemplate(`Hello World`), templateOnly())).toBeObject();

// @ts-expect-error scope is required when strictMode is true
precompileTemplate(`Hello World`, { strictMode: true });

// @ts-expect-error scope must be a function
precompileTemplate(`Hello World`, { strictMode: true, scope: {} });

Expand All @@ -26,6 +23,3 @@ precompileTemplate(`Hello World`, { strictMode: true, scope: () => {} });

// @ts-expect-error scope must return an object and arrays are not the kind of object we want
precompileTemplate(`Hello World`, { strictMode: true, scope: () => [] });

// @ts-expect-error scope has no purpose when strictMode is false
precompileTemplate(`Hello World`, { strictMode: false, scope: () => ({}) });