-
Notifications
You must be signed in to change notification settings - Fork 82
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
Works with Angular12+ (Including 13) #188
base: master
Are you sure you want to change the base?
Conversation
"@angular/common": ">=18.0.0", | ||
"@angular/core": ">=18.0.0", |
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.
Hello @henkkelder
Great job and my thanks for the update, however I have a question: Are you sure it should be 18+?
Because this package is not suitable for angular from 12 to 18 version
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.
Your question confuses me. I am using it myself with Angular 18.
So what do you mean by 'not suitable for angular from 12 to 18'?
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.
Yes, you can use version 18, but for example I am currently working with 16 and your changes will not work for me, since the dependencies include version 18 of Angular. However, in the readme you wrote that the supported version is 12+
With this merge request you are not changing your library, but the global one, which is used by other users besides you and me.
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.
Ah you are right. Angular 18 introduced some breaking changes in Angular components that made it neccesary to update the dependencies.
I have just updated the readme to reflect this.
Are you maintaining this repository now?
@@ -1,14 +1,16 @@ | |||
{ | |||
"name": "ng2-signalr", | |||
"version": "12.0.0", | |||
"name": "@henkkelder/ng2-signalr", |
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.
"name": "@henkkelder/ng2-signalr", | |
"name": "ng2-signalr", |
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.
This will only work if this is actually merged into this repos.
Since my first change was from 2021 and nothing happened with my PR I needed a workable solution.
This version is based in the 'Update to angular 12'. That version however did not work for me since I ran into 'No provider for NgZone' error.
I added CommonModule as a dependency. And that seems to do the trick.