-
Notifications
You must be signed in to change notification settings - Fork 106
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Update Dependency Versions #227
Conversation
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've now updated all dependencies to the current version. In addition I've removed core-js
whose functionality seems to be provided by angular cli directly now.
@earshinov it would be nice if you could give this PR a short test. If everything is ok I will merge it and do a release afterwards. I've also added some comments with some thoughts of mine.
"tslib": "^1.10.0", | ||
"tslint": "~5.18.0", | ||
"typescript": "~3.5.3", | ||
"zone.js": "~0.9.1" | ||
}, | ||
"peerDependencies": { | ||
"@angular/common": ">= 5" |
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.
@earshinov do you know if this is still valid?
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.
No, I did not test angular-archwizard with an old Angular. In our project, we are constantly updating.
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've tested the PR against the demo repository and it appears to work as expected. So it is working for angular 7 at least. I can't tell for older versions.
@@ -24,7 +24,7 @@ import {WizardComponent} from './wizard.component'; | |||
}) | |||
class WizardTestComponent { | |||
|
|||
@ViewChild(WizardComponent) | |||
@ViewChild(WizardComponent, {static: 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.
For now I added to all @ViewChild
and @ContentChild
decorators {static: false}
. I'm not sure if this is the best configuration for all our @XChild
decorators or if there are some where it makes more sense to use {static: true}
instead. What do you think @earshinov?
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.
@madoar , Using { static: false }
by default is the right choice. One of the reasons is that it will be the default in Angular 9. There is more information is the official guide: https://angular.io/guide/static-query-migration#how-do-i-choose-which-static-flag-value-to-use-true-or-false
"rxjs": "~6.5.2", | ||
"scss-bundle": "^2.5.1", | ||
"ts-node": "~8.3.0", | ||
"tsickle": "^0.36.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.
Just a note. yarn install
reports a warning
warning " > tsickle@0.36.0" has incorrect peer dependency "typescript@~3.4.1".
but ng build
runs fine.
There is an open issue in tsickle
: angular/tsickle#1048
Everything seems fine to me. Installation succeeds, build works, tests pass, angular-archwizard-demo runs just fine. I don't see any reasons against merging this MR. |
This PR updates the used dependencies