-
Notifications
You must be signed in to change notification settings - Fork 2
Breaking Change: Update to v19 #1178
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
Conversation
Test Results53 tests 53 ✅ 13s ⏱️ Results for commit 85c78e1. ♻️ This comment has been updated with latest results. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1178 +/- ##
=======================================
Coverage 99.67% 99.67%
=======================================
Files 49 49
Lines 607 607
Branches 38 38
=======================================
Hits 605 605
Misses 2 2 ☔ View full report in Codecov by Sentry. |
| selector: 'hda-host', | ||
| template: ' <hda-model-json-editor [(modelJson)]="modelJson"> </hda-model-json-editor> ' | ||
| template: ' <hda-model-json-editor [(modelJson)]="modelJson"> </hda-model-json-editor> ', | ||
| standalone: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any harm in making them standalone?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there should be any issue in making them standalone, the migration scripts added the standalone false, so I edited the tests accordingly, should I make them standalone?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so, rather than introducing the warning. Doesn't look like too many places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure.
Will update the same
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aaron-steinfeld should we do it in two PRs/versions? Making some components standalone could break things in t-ui.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Separate PR is fine, but it shouldn't break things in t-ui since we can still provide the modules to make it backwards compatible.
I was suggesting to do it in this one in case they did though, since this was already a breaking change it would be better to make both breaking changes at once.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let us do it in a follow-up, mainly because if we migrate to standalone, it could force us to potentially make more changes when we migrate t-ui. We can do it step by step.
| "@angular/common": "^19.0.5", | ||
| "@angular/core": "^19.0.5", | ||
| "@angular/platform-browser": "^19.0.5", | ||
| "@angular-devkit/build-angular": "^19.0.6", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is the version for build-angular different than other angular packages? Did we change it manually or through he ng update command?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we updated it through the ng update command itself
|
Will update the components to be standalone in a follow-up PR |
|
🎉 This PR is included in version 7.1.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Update to Angular v19