-
-
Notifications
You must be signed in to change notification settings - Fork 5
SF-3483 Upgrade to Application Builder and Angular 19 #3338
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
base: master
Are you sure you want to change the base?
Conversation
I intend for this PR to be merged as a single commit, with the PR title as subject, and no commit body. This moves from Angular browser builder and webpack to application builder and vite [1], upgrades from Angular 18 to 19 [2], and updates SASS usage [3]. The Angular output path changes from tsconfig.json
text.component.spec.ts
package.json
SIL.XForge.Scripture.csproj
[1] https://angular.dev/tools/cli/build-system-migration package.json "webpack-bundle-analyzer" may no longer be relevant. In Startup.cs, additional DevelopmentSpaGetRoutes were needed. But it didn't seem to be as important for production routes. We may find that additional routes need added for production. Files such as the following are generated in a production build.
This PR does not change the karma test builder to use application builder ( https://angular.dev/tools/cli/build-system-migration#new-features ), which can be done by setting in angular.json: "test": {
"builder": "@angular-devkit/build-angular:karma",
"options": {
+ "builderMode": "application", This PR introduces unit test failures to lynx-insight-overlay.service.spec.ts, such as to test "should not scroll editor when overlay is already within editor bounds". This error says
It might be fixable for the test by using In event-metrics-log.component.spec.ts, test 'should page event metrics' was broken since nextButton nativeElement.disabled was not true. Investigation found that even when running the application, the element disabled was not set to true. However, aria-disabled changed from null to true. I updated the test to examine aria-disabled rather than disabled. And I adjusted a couple other tests to use the same. We could alternatively drop our check of next button being disabled. Or we could check the paginator Previously, the behaviour in production was that the paginator next button .disabled would be true, and ariaDisabled would be null. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3338 +/- ##
==========================================
- Coverage 82.08% 82.04% -0.04%
==========================================
Files 611 611
Lines 36314 36309 -5
Branches 5990 5987 -3
==========================================
- Hits 29807 29791 -16
- Misses 5642 5659 +17
+ Partials 865 859 -6 ☔ View full report in Codecov by Sentry. |
Although this did not seem to be a problem for the server when running Storybook tests, locally I needed to order 'embedded' staticDirs elements higher in the list for them to be available from Storybook, as seen by testing asset availability with a command such as curl --head http://localhost:6006/assets/audio/test-audio-player.webm http://localhost:6006/assets/images/sf.svg |
If helpful when reviewing, the beginning part of the migration work can be performed with commands: ng update --create-commits @angular/core@19 @angular/cli@19 @angular/pwa@19
# (Press Enter to use "use-application-builder".
# Press Space then Enter to use "provide-initializer".)
$ ng update --create-commits @angular/material@19
# The result can be compared to the commit with title "@angular/material migration - migration-v19", such as with
$ git diff FOO_SHA |
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 some minor nitty things.
I've flagged a lot of missing standalone: true
values. The only reason I raise them is that they are sometimes moved to the end of the @Component
declaration, other times omitted. I don't mind whether we choose to always omit if they are standalone: true
, or whether we should always declare the standalone
value. (I chose to flag the omissions rather than the moves, to make it easier if you do decide to not omit standalone: true
, as its easier to search for standalone: true
if you decide we should omit by default.) My only thought is perhaps always declaring is helpful during this transition period as Angular has changed its default behaviour for standalone
in this release? But up to you which way we go!
Reviewed 201 of 202 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 27 unresolved discussions (waiting on @marksvc)
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/training-data/training-data-multi-select.component.ts
line 24 at r2 (raw file):
selector: 'app-training-data-multi-select', templateUrl: './training-data-multi-select.component.html', standalone: true,
Was this supposed to be removed, or just moved to the end of the block as is done in other files?
Code quote:
standalone: true,
src/SIL.XForge.Scripture/ClientApp/src/app/event-metrics/event-metric-dialog.component.ts
line 13 at r2 (raw file):
templateUrl: './event-metric-dialog.component.html', styleUrls: ['./event-metric-dialog.component.scss'], standalone: true,
Was this supposed to be removed, or just moved to the end of the block as is done in the other files?
Code quote:
standalone: true,
src/SIL.XForge.Scripture/ClientApp/src/app/shared/notice/notice.component.ts
line 10 at r2 (raw file):
templateUrl: './notice.component.html', styleUrls: ['./notice.component.scss'], standalone: true,
Was this supposed to be removed, or just moved to the end of the block as is done in other files?
Code quote:
standalone: true,
src/SIL.XForge.Scripture/ClientApp/src/app/shared/diagnostic-overlay/diagnostic-overlay.component.ts
line 24 at r2 (raw file):
templateUrl: './diagnostic-overlay.component.html', styleUrl: './diagnostic-overlay.component.scss', standalone: true,
Was this supposed to be removed, or just moved to the end of the block as is done in other files?
Code quote:
standalone: true,
src/SIL.XForge.Scripture/ClientApp/src/app/shared/working-animated-indicator/working-animated-indicator.component.ts
line 8 at r2 (raw file):
templateUrl: './working-animated-indicator.component.html', styleUrls: ['./working-animated-indicator.component.scss'], standalone: true,
Was this supposed to be removed, or just moved to the end of the block as is done in other files?
Code quote:
standalone: true,
src/SIL.XForge.Scripture/ClientApp/package.json
line 8 at r2 (raw file):
"scripts": { "ng": "ng", "start": "echo 'open your browser on http://localhost:4200/ '; ng serve",
This command doesn't work on Windows. Does this command work on Linux? (it works on Windows)
"start": "echo \"open your browser on http://localhost:4200/\" && ng serve"
Here is the code that requires the line: https://github.com/dotnet/aspnetcore/blob/main/src/Middleware/Spa/SpaServices.Extensions/src/AngularCli/AngularCliMiddleware.cs#L77
Code quote:
"start": "echo 'open your browser on http://localhost:4200/ '; ng serve",
src/SIL.XForge.Scripture/ClientApp/package.json
line 163 at r2 (raw file):
"ts-mockito": "^2.6.1", "ts-node": "^10.7.0", "typescript": "~5.7.3",
Should we upgrade typescript in RealtimeServer as well, or delay that until a later PR?
Code quote:
"typescript": "~5.7.3",
src/SIL.XForge.Scripture/ClientApp/src/app/shared/audio-recorder-dialog/audio-recorder-dialog.component.ts
line 50 at r2 (raw file):
@Component({ standalone: true,
Was this supposed to be removed, or just moved to the end of the block as is done in other files?
Code quote:
standalone: true,
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-history-list/draft-history-list.component.ts
line 16 at r2 (raw file):
@Component({ selector: 'app-draft-history-list', standalone: true,
Was this supposed to be removed, or just moved to the end of the block as is done in other files?
Code quote:
standalone: true,
src/SIL.XForge.Scripture/Startup.cs
line 342 at r2 (raw file):
) { int periodIndex = path.IndexOf("-");
NIT: Can we rename this to perhaps hashIndex
or fingerprintIndex
?
Code quote:
periodIndex
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/confirm-sources/confirm-sources.component.ts
line 23 at r2 (raw file):
@Component({ selector: 'app-confirm-sources', standalone: true,
Was this supposed to be removed, or just moved to the end of the block as is done in other files?
Code quote:
standalone: true,
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/language-codes-confirmation/language-codes-confirmation.component.ts
line 17 at r2 (raw file):
@Component({ selector: 'app-language-codes-confirmation', standalone: true,
Was this supposed to be removed, or just moved to the end of the block as is done in other files?
Code quote:
standalone: true,
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation.component.ts
line 55 at r2 (raw file):
templateUrl: './draft-generation.component.html', styleUrls: ['./draft-generation.component.scss'], standalone: true,
Was this supposed to be removed, or just moved to the end of the block as is done in other files?
Code quote:
standalone: true,
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-preview-books/draft-preview-books.component.ts
line 46 at r2 (raw file):
templateUrl: './draft-preview-books.component.html', styleUrls: ['./draft-preview-books.component.scss'], standalone: true,
Was this supposed to be removed, or just moved to the end of the block as is done in other files?
Code quote:
standalone: true,
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation-steps/draft-generation-steps.component.ts
line 54 at r2 (raw file):
templateUrl: './draft-generation-steps.component.html', styleUrls: ['./draft-generation-steps.component.scss'], standalone: true,
Was this supposed to be removed, or just moved to the end of the block as is done in other files?
Code quote:
standalone: true,
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation.component.spec.ts
line 44 at r2 (raw file):
let mockDraftGenerationService: jasmine.SpyObj<DraftGenerationService>; let mockDraftSourcesService: jasmine.SpyObj<DraftSourcesService>; const mockDraftHandlingService: jasmine.SpyObj<DraftHandlingService> | undefined = undefined;
NIT: Should this and the other const declarations just be const mockProjectService = undefined;
, as they do not change?
You could also just update the { provide: SFProjectService, useValue: mockProjectService },
to be:
{ provide: SFProjectService, useValue: undefined },
Code quote:
const mockDraftHandlingService: jasmine.SpyObj<DraftHandlingService> | undefined = undefined;
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/supported-back-translation-languages-dialog/supported-back-translation-languages-dialog.component.ts
line 11 at r2 (raw file):
@Component({ selector: 'app-supported-back-translation-languages-dialog', standalone: true,
Was this supposed to be removed, or just moved to the end of the block as is done in other files?
Code quote:
standalone: true,
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-history-list/draft-history-entry/draft-history-entry.component.ts
line 42 at r2 (raw file):
@Component({ selector: 'app-draft-history-entry', standalone: true,
Was this supposed to be removed, or just moved to the end of the block as is done in other files?
Code quote:
standalone: true,
src/SIL.XForge.Scripture/ClientApp/src/app/shared/book-multi-select/book-multi-select.component.ts
line 23 at r2 (raw file):
selector: 'app-book-multi-select', templateUrl: './book-multi-select.component.html', standalone: true,
Was this supposed to be removed, or just moved to the end of the block as is done in other files?
Code quote:
standalone: true,
src/SIL.XForge.Scripture/ClientApp/src/app/translate/biblical-terms/biblical-terms.component.ts
line 177 at r2 (raw file):
templateUrl: './biblical-terms.component.html', styleUrls: ['./biblical-terms.component.scss'], standalone: true,
Was this supposed to be removed, or just moved to the end of the block as is done in other files?
Code quote:
standalone: true,
src/SIL.XForge.Scripture/ClientApp/src/app/translate/font-unsupported-message/font-unsupported-message.component.ts
line 15 at r2 (raw file):
@Component({ selector: 'app-font-unsupported-message', standalone: true,
Was this supposed to be removed, or just moved to the end of the block as is done in other files?
Code quote:
standalone: true,
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-apply-dialog/draft-apply-dialog.component.ts
line 39 at r2 (raw file):
@Component({ selector: 'app-draft-apply-dialog', standalone: true,
Was this supposed to be removed, or just moved to the end of the block as is done in other files?
Code quote:
standalone: true,
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-information/draft-information.component.ts
line 10 at r2 (raw file):
@Component({ selector: 'app-draft-information', standalone: true,
Was this supposed to be removed, or just moved to the end of the block as is done in other files?
Code quote:
standalone: true,
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-usfm-format/draft-usfm-format.component.ts
line 39 at r2 (raw file):
@Component({ selector: 'app-draft-usfm-format', standalone: true,
Was this supposed to be removed, or just moved to the end of the block as is done in other files?
Code quote:
standalone: true,
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-download-button/draft-download-button.component.ts
line 18 at r2 (raw file):
templateUrl: './draft-download-button.component.html', styleUrls: ['./draft-download-button.component.scss'], standalone: true,
Was this supposed to be removed, or just moved to the end of the block as is done in other files?
Code quote:
standalone: true,
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-sources/draft-sources.component.ts
line 57 at r2 (raw file):
@Component({ selector: 'app-draft-sources', standalone: true,
Was this supposed to be removed, or just moved to the end of the block as is done in other files?
Code quote:
standalone: true,
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/training-data/training-data-upload-dialog.component.ts
line 33 at r2 (raw file):
selector: 'app-training-data-upload-dialog', templateUrl: './training-data-upload-dialog.component.html', standalone: true,
Was this supposed to be removed, or just moved to the end of the block as is done in other files?
Code quote:
standalone: 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.
Hmm. So I see that in commit "@angular/core migration - explicit-standalone-flag" it says "... and removes 'standalone:true' from those who are standalone". So because the migration tool itself removed the standalone: true
lines, I'd be inclined to go with that. But I see what you mean about adding clarity during the transition by leaving them in.
I think the existing standalone: true
lines may have been from my fixing problems by changing standalone: false
to true in various places. Looks like they can just be removed, so I am doing that.
I tried removing standalone: true
from import-questions-dialog.component.html but that causes problems.
Removing standalone: true
from a couple pipe.ts files did not seem to be a problem in tests or working with the application; I have done that.
Reviewable status: all files reviewed, 27 unresolved discussions (waiting on @pmachapman)
src/SIL.XForge.Scripture/Startup.cs
line 342 at r2 (raw file):
Previously, pmachapman (Peter Chapman) wrote…
NIT: Can we rename this to perhaps
hashIndex
orfingerprintIndex
?
Ha. Yes. Good catch. How about hashDelimiterIndex
since it's the index of the delimiter, not necessarily the hash?
src/SIL.XForge.Scripture/ClientApp/package.json
line 8 at r2 (raw file):
Previously, pmachapman (Peter Chapman) wrote…
This command doesn't work on Windows. Does this command work on Linux? (it works on Windows)
"start": "echo \"open your browser on http://localhost:4200/\" && ng serve"Here is the code that requires the line: https://github.com/dotnet/aspnetcore/blob/main/src/Middleware/Spa/SpaServices.Extensions/src/AngularCli/AngularCliMiddleware.cs#L77
Ah! Thank you! Yes.
Though in my testing, I need a space at the end of the string or it does not work. (The frontend shows a white screen saying "Loading".)
src/SIL.XForge.Scripture/ClientApp/package.json
line 163 at r2 (raw file):
Previously, pmachapman (Peter Chapman) wrote…
Should we upgrade typescript in RealtimeServer as well, or delay that until a later PR?
Oo, good question. I am a bit split about it. I'll update it here in RealtimeServer as well.
src/SIL.XForge.Scripture/ClientApp/src/app/event-metrics/event-metric-dialog.component.ts
line 13 at r2 (raw file):
Previously, pmachapman (Peter Chapman) wrote…
Was this supposed to be removed, or just moved to the end of the block as is done in the other files?
It was removed by the migration tool. I explain more in a another comment.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation.component.spec.ts
line 44 at r2 (raw file):
Previously, pmachapman (Peter Chapman) wrote…
NIT: Should this and the other const declarations just be
const mockProjectService = undefined;
, as they do not change?You could also just update the
{ provide: SFProjectService, useValue: mockProjectService },
to be:{ provide: SFProjectService, useValue: undefined },
Sure. That looks cleaner.
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 20 of 20 files at r3, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @marksvc)
The testers found a problem where an error dialog is shown about localization. |
@pmachapman When you have a moment, could you code review the changes since last time? Thank you! |
|
Storybook was giving errors like > node_modules/webpack/types.d.ts:253:4 - error TS2315: Type 'Uint8Array' is not generic.
Removed `static` from `SpaGetRoutes` and `SpaPostRoutes` because their modification was lasting between test instantiations.
This change is