-
-
Notifications
You must be signed in to change notification settings - Fork 5
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
SF-3030 Update to Angular 18 #2804
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2804 +/- ##
==========================================
+ Coverage 79.06% 79.07% +0.01%
==========================================
Files 531 532 +1
Lines 30793 30794 +1
Branches 5028 5033 +5
==========================================
+ Hits 24347 24351 +4
- Misses 5661 5672 +11
+ Partials 785 771 -14 ☔ View full report in Codecov by Sentry. |
ede4670
to
58daa69
Compare
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.
Reviewable status: 0 of 24 files reviewed, 1 unresolved discussion (waiting on @kylebuss)
src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-comments/checking-comments.stories.ts
line 4 at r1 (raw file):
import { Meta, moduleMetadata, StoryObj } from '@storybook/angular'; import { expect } from '@storybook/test'; import { within } from '@storybook/test';
I'm surprised this didn't get auto-formatted on save to only import once, and I'm seeing a lot of reformatting that doesn't seem necessary.
58daa69
to
9179984
Compare
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.
Reviewable status: 0 of 24 files reviewed, 1 unresolved discussion (waiting on @Nateowami)
src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-comments/checking-comments.stories.ts
line 4 at r1 (raw file):
Previously, Nateowami wrote…
I'm surprised this didn't get auto-formatted on save to only import once, and I'm seeing a lot of reformatting that doesn't seem necessary.
I'll fix the import statements. The update combined @storybook/jest
and @storybook/testing-library
into @storybook/test
which is why some of the stories looked like this.
The automerge tool also did the reformatting you're observing. The only file I had to fix was the font-size.stories.ts
.
What's the automerge tool? |
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.
Sorry, automigrate npx storybook@latest automigrate
which ran when I performed the update npx storybook@latest upgrade
.
Reviewable status: 0 of 24 files reviewed, 1 unresolved discussion (waiting on @Nateowami)
9179984
to
8096d8e
Compare
8096d8e
to
233520e
Compare
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.
Reviewed 14 of 24 files at r1, 8 of 8 files at r2, 68 of 68 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @kylebuss and @Nateowami)
src/SIL.XForge.Scripture/ClientApp/tsconfig.json
line 10 at r3 (raw file):
"strictNullChecks": false, "noImplicitAny": false, "noImplicitReturns": false,
Any reason for turning off no implicit returns?
Code quote:
"noImplicitReturns": false,
src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-comments/checking-comments.component.html
line 2 at r3 (raw file):
<ng-container *transloco="let t; read: 'checking_comments'"> @for (comment of getSortedComments(); track $index; let i = $index) {
I read the @for
article about duplicate keys. Is that what is going on here? Wouldn't it be better to track a unique property within a comment? Such as comment.dataId?
Code quote:
@for (comment of getSortedComments(); track $index; let i = $index) {
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.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Nateowami and @RaymondLuong3)
src/SIL.XForge.Scripture/ClientApp/tsconfig.json
line 10 at r3 (raw file):
Previously, RaymondLuong3 (Raymond Luong) wrote…
Any reason for turning off no implicit returns?
Yes, we have 25-30 functions that throw error TS7030: Not all code paths return a value.
because they have return;
. I meant to say something about this as I didn't know if it was in scope for this change or something we want to address separately.
src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-comments/checking-comments.component.html
line 2 at r3 (raw file):
Previously, RaymondLuong3 (Raymond Luong) wrote…
I read the
@for
article about duplicate keys. Is that what is going on here? Wouldn't it be better to track a unique property within a comment? Such as comment.dataId?
I think normally, yes, using comment.dataId
would be preferred. However, because we are using let i = $index
and i
is used in the loop I think it makes more sense to track on $index
here.
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.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @kylebuss and @RaymondLuong3)
src/SIL.XForge.Scripture/ClientApp/tsconfig.json
line 10 at r3 (raw file):
Previously, kylebuss (Kyle Buss) wrote…
Yes, we have 25-30 functions that throw
error TS7030: Not all code paths return a value.
because they havereturn;
. I meant to say something about this as I didn't know if it was in scope for this change or something we want to address separately.
I assume they could be changed to return undefined;
? It should be easy to do a global find and replace of return;
to return undefined;
(if we want to do that).
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.
Nice work getting past all the hurdles to make this upgrade. Well done. I just noticed a small UI related glitch related to the offline icon. It should be an easy fix. I think we should send this to testers for a surface level inspection.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @kylebuss)
src/SIL.XForge.Scripture/ClientApp/tsconfig.json
line 10 at r3 (raw file):
Previously, Nateowami wrote…
I assume they could be changed to
return undefined;
? It should be easy to do a global find and replace ofreturn;
toreturn undefined;
(if we want to do that).
That's reasonable. I'm OK either we make that change or not.
src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-comments/checking-comments.component.html
line 2 at r3 (raw file):
Previously, kylebuss (Kyle Buss) wrote…
I think normally, yes, using
comment.dataId
would be preferred. However, because we are usinglet i = $index
andi
is used in the loop I think it makes more sense to track on$index
here.
That is probably reasonable to do that. I haven't seen track $index
until this PR. I'll keep note of that possiblity.
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.
Good catch. There is a good chance there's a number of little things like this we will start finding!
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @kylebuss)
src/SIL.XForge.Scripture/ClientApp/tsconfig.json
line 10 at r3 (raw file):
Previously, RaymondLuong3 (Raymond Luong) wrote…
That's reasonable. I'm OK either we make that change or not.
I can make the change now. I was thinking it was in a lot more places originally.
233520e
to
456285f
Compare
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.
Resolved noImplicitReturns
errors and offline indicator display.
Reviewable status: 87 of 102 files reviewed, all discussions resolved (waiting on @RaymondLuong3)
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.
Reviewable status: 87 of 102 files reviewed, all discussions resolved (waiting on @RaymondLuong3)
src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-comments/checking-comments.component.html
line 2 at r3 (raw file):
Previously, RaymondLuong3 (Raymond Luong) wrote…
That is probably reasonable to do that. I haven't seen
track $index
until this PR. I'll keep note of that possiblity.
Isn't it potentially possible for the a comment to come over the realtime server that change the index of some of the comments, making them no longer track correctly?
456285f
to
21bd82b
Compare
Previously, Nateowami wrote…
Good point. I'd rather not risk finding out if this is a possibility. It looks like |
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.
Reviewed 15 of 15 files at r4, 3 of 3 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @kylebuss)
src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-comments/checking-comments.component.html
line 2 at r3 (raw file):
Previously, kylebuss (Kyle Buss) wrote…
Good point. I'd rather not risk finding out if this is a possibility. It looks like
$index
should only be used on static collections, so I updated/reverted the loops that I could to not use it.
From my understanding, the index of a comment would not change since we are getting the comments sorted... well if a user created a comment offline and the come online, then the index of the comments could change. Ok, let's stick with the existing behaviour and not do too much on update PRs like this.
I am still seeing the offline icon not being positioned incorrectly. That should be a quick fix, so I'll leave this in the ready to test column. |
21bd82b
to
62387e1
Compare
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.
@RaymondLuong3, not sure what I was looking at before as I should have compared to QA. The offline indicator should now be showing in the correct spot.
Reviewable status: 100 of 102 files reviewed, all discussions resolved (waiting on @RaymondLuong3)
62387e1
to
f9d3f16
Compare
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, the test you added does not actually test the code you added in ParatextService.
Reviewed 3 of 3 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @kylebuss)
This PR provides updates to Angular 18. When pulling/merging this request
npm ci
will need to be ran in the console for both ClientApp.@storybook/test
and@storybook/testing-library
are not combined into@storybook/test
and a number of spec files required updates toimport
andproviders
this update followed the guide here.This update has introduced control flow warnings on
@for
loops to ensure tracking is done on a unique key. More can be read here.Angular Material adds support for M3, to maintain M2 compatibility this update required a few
.scss
changes. To learn more about material functions here's the guide I used.Additional packages/dependencies that were updated to be compatible with Angular 18:
This change is