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

fix(vite-plugin-angular): correctly implement HMR of component styles #1542

Merged
merged 6 commits into from
Jan 7, 2025

Conversation

mattlewis92
Copy link
Contributor

PR Checklist

Follow up to #1539

What is the new behavior?

In #1539 I thought I had fixed HMR, but actually what that change did was disable HMR for component stylesheets and trigger a full reload 🤦‍♂️

So this followup implements the missing pieces:

  1. Implements the appropriate callback for component stylesheets in the handleHotUpdate plugin hook to reimplement HMR of component styles
  2. Handles ViewEncapsulation.Emulated components by using the encapsulateStyle helper

Does this PR introduce a breaking change?

  • Yes
  • No

Relevant bits from the CLI where I based this code from:

Other information

[optional] What gif best describes this PR or how it makes you feel?

Copy link

netlify bot commented Jan 6, 2025

Deploy Preview for analog-app ready!

Name Link
🔨 Latest commit 691ee70
🔍 Latest deploy log https://app.netlify.com/sites/analog-app/deploys/677d30c411bbf20008258e93
😎 Deploy Preview https://deploy-preview-1542--analog-app.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Jan 6, 2025

Deploy Preview for analog-blog ready!

Name Link
🔨 Latest commit 691ee70
🔍 Latest deploy log https://app.netlify.com/sites/analog-blog/deploys/677d30c4ba3e2e0008203293
😎 Deploy Preview https://deploy-preview-1542--analog-blog.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Jan 6, 2025

Deploy Preview for analog-docs ready!

Name Link
🔨 Latest commit 691ee70
🔍 Latest deploy log https://app.netlify.com/sites/analog-docs/deploys/677d30c49a431e0008819d90
😎 Deploy Preview https://deploy-preview-1542--analog-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Jan 6, 2025

Deploy Preview for analog-ng-app ready!

Name Link
🔨 Latest commit 691ee70
🔍 Latest deploy log https://app.netlify.com/sites/analog-ng-app/deploys/677d30c42f8a03000881fbe3
😎 Deploy Preview https://deploy-preview-1542--analog-ng-app.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@@ -14,6 +14,7 @@ import {
ResolvedConfig,
Connect,
} from 'vite';
import { encapsulateStyle } from '@angular/compiler';
Copy link
Member

Choose a reason for hiding this comment

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

Do you know what version this was introduced in? We support Angular v17-19

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like it was added in 17.1.0: angular/angular@f0377d3

I can just make it lazy as we only need it if liveReload is enabled, which will only work in v19+

Copy link
Member

Choose a reason for hiding this comment

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

Good idea

Copy link
Contributor Author

@mattlewis92 mattlewis92 Jan 6, 2025

Choose a reason for hiding this comment

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

@brandonroberts
Copy link
Member

@mattlewis92 something has broken with the styles in the PR preview

https://deploy-preview-1542--analog-app.netlify.app/

image

https://analog-app.netlify.app/

image

@mattlewis92
Copy link
Contributor Author

@mattlewis92 something has broken with the styles in the PR preview

deploy-preview-1542--analog-app.netlify.app

image

analog-app.netlify.app

image

I think this should fix it! 9ca000a (#1542)

@brandonroberts
Copy link
Member

@mattlewis92 that does fix the build, but the development server is still the same if you run nx serve analog-app and go to http://localhost:3000

@brandonroberts
Copy link
Member

I think it was this PR that introduced the regression #1539

@mattlewis92
Copy link
Contributor Author

I think it was this PR that introduced the regression #1539

Yep it's definitely that. The problem seems to be specific to component inline styles rather than external styleUrls

The vite plugin inlines the style into the href of the stylesheet:
CleanShot 2025-01-07 at 11 00 03@2x

Whereas the angular cli creates a virtual file that contains the compiled css:

CleanShot 2025-01-07 at 11 00 13@2x

@mattlewis92
Copy link
Contributor Author

OK this should do the trick! 691ee70 (#1542)

@brandonroberts
Copy link
Member

@mattlewis92 thanks! Will you add this back in this PR? #1539

I reverted it on beta so we can keep it all as one change

@brandonroberts brandonroberts merged commit 5ec113f into analogjs:beta Jan 7, 2025
23 of 24 checks passed
@brandonroberts
Copy link
Member

Thanks again @mattlewis92. Works great!

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.

2 participants