-
Notifications
You must be signed in to change notification settings - Fork 68
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
feat(angular): Upgrade to Angular 5 #38
feat(angular): Upgrade to Angular 5 #38
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #38 +/- ##
===========================================
+ Coverage 99.01% 99.13% +0.11%
===========================================
Files 14 14
Lines 408 460 +52
Branches 65 79 +14
===========================================
+ Hits 404 456 +52
Misses 4 4
Continue to review full report at Codecov.
|
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.
Looks good, just have three small remarks :)
@@ -22,7 +22,7 @@ export class NotifierTimerService { | |||
/** | |||
* Timeout ID, used for clearing the timeout later on | |||
*/ | |||
private timerId: number; | |||
private timer: NodeJS.Timer; |
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 this is an issue introduced by the NodeJS typings, the timer ID here should be of type number
(as before), not NodeJS.Timer
. Perhaps we should cast this into a number, if necessary?
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.
You are right, TypeScript was complaining about this, I didn't check the signature of the method setTimeout which is wrong microsoft/TypeScript#842 I'll use window.setTimeout instead.
package.json
Outdated
"@angular/platform-browser-dynamic": "4.1.x", | ||
"@angular/platform-browser": "4.1.x", | ||
"@types/jest": "20.0.x", | ||
"@angular/common": "^5.0.3", |
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 usually prefer 5.0.x
instead of ^5.0.3
as it is easier to understand for developers
package.json
Outdated
@@ -48,64 +48,60 @@ | |||
"ci:coverage": "codecov -f coverage/coverage-final.json", | |||
"ci:test": "jest --config tools/jest/jest.config.json --runInBand --no-cache" | |||
}, | |||
"peerDependencies": { |
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.
Please do not remove the peerDependencies, instead update to >= 5.0.0 < 6.0.0
@dominique-mueller I've addressed your comments :) |
Thanks again @DavidRouyer for your contribution! I will do some further build-related updates and release v3 of angular-notifier in the next few days. |
BREAKING CHANGE: The upgrade to Angular 5 breaks compatibility with Angular 4.