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

Jump to bottom

[BUGFIX types] Add missing type declarations in the preview types for @ember/template-compilation #20410

Merged
merged 2 commits into from
Mar 22, 2023
Merged

[BUGFIX types] Add missing type declarations in the preview types for @ember/template-compilation #20410

merged 2 commits into from
Mar 22, 2023

Conversation

NullVoxPopuli
Copy link
Contributor

@NullVoxPopuli NullVoxPopuli commented Mar 16, 2023 •
edited
Loading

Adds types for @ember/template-compilation

@NullVoxPopuli NullVoxPopuli changed the title [BUGFIX types] Add missing type declarations in the preview types for… [BUGFIX types] Add missing type declarations in the preview types for @ember/template-compilation Mar 16, 2023
NullVoxPopuli
NullVoxPopuli commented Mar 16, 2023
View reviewed changes
type-tests/@ember/template-compilation.ts
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();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

is there any value in keeping expectTypeOf here since setComponentTemplate is already tested?

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems fine to keep it here as well.

@NullVoxPopuli NullVoxPopuli marked this pull request as ready for review March 16, 2023 15:31
chriskrycho
chriskrycho requested changes Mar 21, 2023
View reviewed changes
Copy link
Contributor

@chriskrycho chriskrycho left a comment

Choose a reason for hiding this comment

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

A couple suggested changes!

chriskrycho
chriskrycho approved these changes Mar 22, 2023
View reviewed changes
Copy link
Contributor

@chriskrycho chriskrycho left a comment

Choose a reason for hiding this comment

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

Thank you!

@chriskrycho chriskrycho merged commit 2217669 into emberjs:master Mar 22, 2023
@NullVoxPopuli NullVoxPopuli deleted the add-ember-template-compilation branch March 22, 2023 14:53
@boris-petrov
Copy link
Contributor

This is missing the compileTemplate function. Is this on purpose?

@NullVoxPopuli
Copy link
Contributor Author

Is it public api for @ember/template-compilation?

It's from a different package: https://github.com/emberjs/ember.js/blob/main/packages/%40ember/template-compilation/index.ts#L4
Which doesn't have docs on api.emberjs.com

@chriskrycho
Copy link
Contributor

It is indeed public API, and in fact is recommended by a different public API:

ember.js/packages/@ember/template-compilation/index.ts

Line 35 in 5de3020

'Attempted to call `precompileTemplate` at runtime, but this API is meant to be used at compile time. You should use `compileTemplate` instead.'

We can probably do a backport for the 4.12 LTS types as well stable; it is not relevant for 5.1 and after, where the types are present by way of compiling from course.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jamescdavis jamescdavis jamescdavis left review comments

@chriskrycho chriskrycho chriskrycho approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@NullVoxPopuli @boris-petrov @chriskrycho @jamescdavis