Upgrade to Angular v14 #109
Conversation
|
/ok-to-test |
|
Please use a rebase to master, not merge from master |
Signed-off-by: Harshit Nayan <[email protected]>
Signed-off-by: LogicalGuy77 <[email protected]>
LogicalGuy77
force-pushed
the
logicalguy-patch-4-angular13
branch
from
c2692e7 to
466fbbb
Compare
|
Fixed😊, #108 is already in this PR so we can close that. |
* testing cidcd * fixed lint Signed-off-by: LogicalGuy77 <[email protected]>
LogicalGuy77
force-pushed
the
logicalguy-patch-4-angular13
branch
from
1b2a067 to
a63c577
Compare
Signed-off-by: Harshit Nayan <[email protected]>
frontend/tsconfig.json
Outdated
| "allowSyntheticDefaultImports": true, | ||
| "skipLibCheck": true, | ||
| "forceConsistentCasingInFileNames": true, | ||
| "strict": false, | ||
| "noImplicitReturns": false, | ||
| "noFallthroughCasesInSwitch": false | ||
| }, | ||
| "angularCompilerOptions": { | ||
| "fullTemplateTypeCheck": true, | ||
| "strictInjectionParameters": true | ||
| "strictInjectionParameters": true, | ||
| "enableIvy": true | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Could you give some explanations for these changes? One that stands out is strict mode. I would prefer to be using that.
There was a problem hiding this comment.
- Main date-fns API change issue:
Error: export 'distanceInWords' (imported as 'distanceInWords') was not found in 'date-fns' - TypeScript errors with Node.js type definitions:
Error: node_modules/@types/node/http.d.ts:230:27 - error TS2362
Changes:
- Added
"skipLibCheck": true- This will tell TypeScript to skip type checking of declaration files (*.d.ts), which will help with the @types/node errors "strictInjectionParameters": trueEnsures type safety when injecting dependencies"enableIvy": true- Enables Angular's Ivy rendering and compilation pipeline (default in Angular 13+)
I'll try removing strict mode to see if build is successful.
There was a problem hiding this comment.
@Griffin-Sullivan removing strict mode runs fine locally, I'll update changes in next commit
|
Mind updating the README instructions to mention the correct Node version? |
Dockerfile
Outdated
|
|
||
| COPY --from=fetch-kubeflow-kubeflow $LIB/ ./ | ||
| # Patch all SCSS files with tilde imports | ||
| RUN find . -name "*.scss" -exec sed -i "s|@use '~@angular/material' as mat;|@use '@angular/material' as mat;|" {} \; |
There was a problem hiding this comment.
This is not necessary if we can get our common lib commit to kubeflow/kubeflow@78969ee#diff-e97c6691e0d4878d4bc25a491f905b11d224fa508aced14c98ef7b4317910ec7R1
There was a problem hiding this comment.
Yes please lets do the upgrade
"testing it now. The whole tilda replacement wouldn't be necessary if we update the COMMIT we are using for the kubeflow common lib. See this commit from a few months ago https://github.com/kubeflow/kubeflow/commit/78969ee8246a6f11295378fc61273b1f2dbe07a8#diff-e97c6691e0d4878d4bc25a491[…]fa508aced14c98ef7b4317910ec7R1"
There was a problem hiding this comment.
I used this COMMIT 78969ee but it image was failing to build. I suspect it is because I've upgraded to Angular 13 and this commit 78969ee uses more recent Angular version. Can we get a commit which is compatible with angular 13?
Or should we change our approach to this problem?
@juliusvonkohout @Griffin-Sullivan
Signed-off-by: LogicalGuy77 <[email protected]>
Signed-off-by: LogicalGuy77 <[email protected]>
…icalGuy77/models-web-app-logical into logicalguy-patch-4-angular13
|
Sorry for only running the workflows now. |
|
To me it seems that the image is building. Does it run locally as well? we should get this PR merged as soon as possible and work in follow-up PRs. |
|
@tarekabouzeid might help as well with testing |
…o 78969ee Signed-off-by: LogicalGuy77 <[email protected]>
Signed-off-by: LogicalGuy77 <[email protected]>
|
I've made changes as per our discussion :) |
|
I see the frontend tests failing in https://github.com/kserve/models-web-app/actions/runs/14085457407/job/39453075493?pr=109. If you fix that, let me know and i can run the workflows and merge if they are green. Then you can move over all open topics to a follow up PR. In that follow up PR the ci/cd and automatic testing will be quite important as well. |
…iguration Signed-off-by: LogicalGuy77 <[email protected]>
|
@juliusvonkohout I've upgraded the frontend tests to follow as per Angular v14, tests are passing. I've checked them on my repo, you can view it here: LogicalGuy77#9 |
|
/lgtm Please move over all open topics to a follow up PR. In that follow up PR the ci/cd and automatic testing will be quite important as well. |
|
and congratulations, this was a significant contribution :-) |
|
Ah and you need to migrate from docker.io to ghcr as well in the next PR. |
|
thank you @juliusvonkohout , happy to have my first PR merged!😊 I'll start with the follow up PRs. I'll create a new slack thread where we can discuss the areas which we'll focus.
|
Upgraded to Angular
v13v14 and fixed other dependencies issues.Related: #108
Testing:
Note: We need to patch all SCSS files in kubeflow repo since while building image we are using components/crud-web-apps/common/frontend.
Currently I am patching it in my Dockerfile
Should I go and create an issue and fix for it in kubeflow repo?
please review @juliusvonkohout , @Griffin-Sullivan