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

Account for string literals in styles and the new styleUrls #25812

Merged
merged 2 commits into from
Sep 13, 2023

Conversation

crisbeto
Copy link
Member

@crisbeto crisbeto commented Sep 9, 2023

Updates the JIT transformers to account for the upcoming changes from angular/angular#51715.

@crisbeto crisbeto added action: review The PR is still awaiting reviews from at least one requested reviewer target: major This PR is targeted for the next major release merge: preserve commits When the PR is merged, a rebase and merge should be performed labels Sep 9, 2023
@@ -143,82 +143,93 @@ function visitComponentMetadata(
switch (node.name.text) {
case 'templateUrl':
// Only analyze string literals
if (
Copy link
Member Author

Choose a reason for hiding this comment

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

Note: I couldn't figure out where this logic is tested so I didn't add any tests. I can add some if somebody can point me in the right direction.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm okay, it looks like I can't write a test there, because the change hasn't landed in a release yet so writing a component with the new properties results in a compilation error.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually even if this compiled, it wouldn't be testing the behavior we're looking for, because the transformer only runs on JIT code. As far as I can tell, these tests are running AOT compilation.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually even if this compiled, it wouldn't be testing the behavior we're looking for, because the transformer only runs on JIT code. As far as I can tell, these tests are running AOT compilation.

You can switch between AOT and JIT by providing the aot as an option in

Let's land it for now, and follow up with a unit test in a separate PR once the changes in the FW are released on NPM.

@crisbeto crisbeto marked this pull request as draft September 11, 2023 07:27
@crisbeto crisbeto changed the title Account for string literals in styleUrls and styles Account for string literals in styles and the new styleUrls Sep 11, 2023
…s and styleUrl

An upcoming change in Angular will allow `style` specified as strings, in addition to a new `styleUrl` property. These changes update the Webpack transform to support the change.
…tring literals and styleUrl

An upcoming change in Angular will allow `style` specified as strings, in addition to a new `styleUrl` property. These changes update the JIT resource transform to support the change.
@crisbeto crisbeto marked this pull request as ready for review September 11, 2023 12:40
Copy link
Collaborator

@alan-agius4 alan-agius4 left a comment

Choose a reason for hiding this comment

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

LGTM

@crisbeto crisbeto added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Sep 12, 2023
@alan-agius4 alan-agius4 merged commit 2f299fc into angular:main Sep 13, 2023
35 checks passed
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Oct 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker merge: preserve commits When the PR is merged, a rebase and merge should be performed target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants