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

Fixed the issue #49 #53

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

JeevanMahesha
Copy link
Contributor

No description provided.

@JeevanMahesha
Copy link
Contributor Author

@samdenty / @apai4 can you please review this PR.

Copy link
Member

@Nemikolh Nemikolh left a comment

Choose a reason for hiding this comment

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

I still have the same feedback as with the other PR. Is the @angular-devkit/build-angular dependency really necessary?

It pulls in a LOT of dependencies. Checking the size of that dependency only in pkg-size.dev, shows 196MB.

In comparison, the @angular/cli pulls far less. This will impact user experience on the Angular starter quite a bit.

@JeevanMahesha
Copy link
Contributor Author

@Nemikolh sorry I missed to add this comment.

In the latest versions of Angular (18+), the build system has transitioned to an esbuild-based builder as the default (@angular-devkit/build-angular:application). This change significantly reduces dependency overhead and improves build performance. The previous browser and browser-esbuild builders are now deprecated in favor of this unified application builder.

I’ve double-checked the @angular-devkit/build-angular dependency, and it’s still required for leveraging the default Angular CLI build pipeline.

Let me know if you’d prefer, we take alternative routes, or if further testing is needed for the current setup.

@Nemikolh
Copy link
Member

I think, I will leave this decision to someone from the Angular team. @alxhub if you have time to check this, that would be really appreciated.

To reiterate on my concerns, I'm worried that the dependencies added by this PR will impact:

  • The boot time of the angular starter (as downloading those dependencies will slowdown the setup time)
  • The memory usage (and if it's too high the tab might crash for some users)

@michael-small
Copy link

michael-small commented Jan 11, 2025

I just realized I dealt with a similar thing recently. Too similar...

This underlying issue with the builder which prompted me to make the issue that the previous PR was attempting to fix made me remember... it appears to be the same issue as the bug I reported in the angular.dev playground when using in-component styles back in November.

adev(bug): "Learn Angular in the Browser" breaks with styles #28925
angular/angular-cli#28925

fix(@angular/build): use sha256 instead of sha-256 as hash algorithm name #28926
angular/angular-cli#28926

I made a fork of my issue recreation Stackblitz project and bumped it to Angular packages version 19.0.5 like the fix PR tried to do, but without changing away from the @angular/build:application builder: https://stackblitz.com/edit/stackblitz-starters-mbnptpoq?file=package.json

TL;DR

A fix to the @angular/build:application builder was made after I reported basically the same issue but in the Angular repo. This fix went into a later version of Angular than the current 19.0.0 version. I think you can probably just bump to 19.0.6 for now and call it good if I am understanding everything.

@JeevanMahesha
Copy link
Contributor Author

Thanks, @michael-small and @Nemikolh, for your insights and the references to the related issues and fixes. The background you provided was invaluable in identifying the root cause and verifying the solution.

I've bumped the Angular packages version to 19.0.6, as suggested, and confirmed that the issue is resolved with the updated @angular/build:application builder. The necessary changes have been made in this PR.

Could you please review the updates at your convenience?

Thanks again for your collaboration! 🙌

Copy link
Member

@Nemikolh Nemikolh left a comment

Choose a reason for hiding this comment

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

Nice! Thank you both for investigating 🙏

Just a minor thing and we can merge this!

Comment on lines +22 to +23
"@angular/build": "^19.0.7",
"@angular/cli": "^19.0.7",
Copy link
Member

Choose a reason for hiding this comment

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

Let's keep all the dependencies at the same version 👍

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.

3 participants