-
Notifications
You must be signed in to change notification settings - Fork 33
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
Bump dependencies (i.a. Angular 8->10) #124
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.
Tests are passing which means we are good with this upgrade. Please, take a look on comments.
@@ -71,7 +71,6 @@ | |||
"no-switch-case-fall-through": true, | |||
"no-trailing-whitespace": true, | |||
"no-unnecessary-initializer": true, | |||
"no-use-before-declare": true, |
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 rule seems to make sense, why it has been removed?
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.
It got removed together with a Typescript version bump as it is deprecated since 2.9
- see palantir/tslint#4695. The explanation for the removal is here (tl;dr - it isn't useful when you don't use var
keyword, and we don't in the integration code). However, as mentioned in this comment, there are still cases where it may be useful, so I'll bring it back.
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.
Didn't know that, nice research 👍 But if it has been deprecated, maybe we should keep it removed then?
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.
Well, until we fix #30 it doesn't really matter 😄 but sure, let's keep it away while we can and if we encounter a valid use case for it in our code we can restore it.
I admit I didn't pay too much attention to investigate what exactly changes in the repo after bumping the deps, so not all of the changes were relevant - I assumed that Angular knows what it's doing 😞 I was just happy that everything works. So thanks for bringing my attention to the guts 👍 |
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.
Honestly, I didn't know that it has been done by Angular itself, I thought you did it 🙂 Nevertheless, it's good to know what happened. Could you refer to #124 (comment) and propose changelog entry? I suppose that since we've updated package dependencies, it should be reflected in the changelog. There is a lot of them, so maybe it should be some generic, single entry about mainly angular upgrade + related dependencies?
I proposed the changelog entry (updated the first PR comment) - last time we bumped deps we didn't include info about it, and the time before that we did, so there is some inconsistency. I think it would be good to write about it after all but it the same time make users notice that only repo deps change and not package ones. |
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.
LGTM!
Proposed changelog entry
Closes #98.
Closes #116.
Closes #125.
(Also #112 but we are checking how dependabot behaves in such a case).