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

Conversation

ef4
Copy link
Contributor

@ef4 ef4 commented Dec 12, 2023

As of #20587, the modules published under dist/packages act more like normal addon code, in that their templates are published as calls to precompileTemplate rather than wire format.

But that makes this the first time that Embroider is seeing these templates, and because they are in loose mode and use the dynamic component helper they are un-analyzable.

This PR:

  • switches them to strict mode
  • fixes the declared types for precompileTemplate, which were a lie before. scope is allowed in non-strict mode. scope is optional even in strict mode
  • fixes a spelling error in a recently-introduced internal method, because I noticed it and it annoyed me.

As of #20587, the modules published under `dist/packages` act more like normal addon code,  in that their templates are published as calls to `precompileTemplate` rather than wire format.

But that makes this the first time that Embroider is seeing these templates, and because they are in loose mode and use the dynamic component helper they are un-analyzable.

This PR:
 - switches them to strict mode
 - fixes the declared types for `precompileTemplate`, which were a lie before. `scope` is allowed in non-strict mode. `scope` is optional even in strict mode.
 - fixes a spelling error in a recently-introduced internal method, because I noticed it and it annoyed me.
@@ -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?

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.

@@ -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.

@ef4
Copy link
Contributor Author

ef4 commented Dec 12, 2023

This PR will be necessary but probably not sufficient to get the ember-cli embroider scenario passing.

I also see an issue around test setup evaluation order that I'm investigating now.

@@ -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.

@ef4
Copy link
Contributor Author

ef4 commented Dec 12, 2023

The other issue is embroider-build/embroider#1717

@ef4 ef4 merged commit a93e12c into main Dec 12, 2023
21 checks passed
@ef4 ef4 deleted the strict-internal-templates branch December 12, 2023 23:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants