Skip to content
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

350 framework lifecycle #357

Merged
merged 26 commits into from
Feb 9, 2022
Merged

350 framework lifecycle #357

merged 26 commits into from
Feb 9, 2022

Conversation

mburri
Copy link
Contributor

@mburri mburri commented Feb 9, 2022

  • Upgrade von Angular 11 auf Angular 13
  • Upgrade von RxJS 6.x auf 7.x
    • Dieses Upgrade erforderte einige Anpassnungen am Code - in den meisten Fällen musste ich dem Subject destroy$' den generischen Typ hinzfügen - weil sonstdestroy$.next()` nicht compiliert.
  • Upgrade aller sonstiger Dependencies - ausser Bootstrap. Unsere Version von ng-boostrap basiert auf Boostrap 4 (wir verwenden die aktuelleste Version von ng-bootstrap). Eine erste Analyse hat gezeigt, dass die Migration auf Boostrap 5 aufwändig werden wird, da doch wiederum einiges geändert hat.

Mit Angular 13 kann nicht mehr tslint verwendet werden - darum machen wir das Linting neu mit eslint - statt einer spezifischen Konfiguration für die Webapp-Schulverwaltung habe ich im die default/ recommended Settings für Angular genommen. Zusätzlich (wie bisher) ergänzt mit Prettier.
Die neuen eslint Rules haben dann einige Fehler geworfen:

  • LifeCycle Methoden (onIinit()) dürfen nicht mehr leer sein.
  • Negieren von Async Pipes ist nicht erlaubt, zum Beispiel: *ngIf="!(state.loading$ | async);

Mit Angular 13 kommt auch ein neuer Sass Compiler - Division mit Slash \ ist deprecated - mittels Migrationsstool konnten diese Warnings automatisch behoben werden.

Node wurde ebenfalls auf die neueste Version aktualisiert (lts/gallium)

mburri added 26 commits February 9, 2022 11:44
Angular 13 requires at least version 12.2.

Upgrading node seems to break lint-staged. I upgraded that too and changed to pre-commit hook.
Angular 11 or one of the current dependency does not work with Node 16 - I postpone this upgrade for now and upgrade Angular and other dependencies first
slash.div divisions have been deprecated: https://sass-lang.com/documentation/breaking-changes/slash-div

this commit is the result of running sass-migrator division for all files that emmited a warning. The migrator script replaces divisions by 2 (/ 2) with a multiplication (* 0.5) - this yields the same result but was a bit unecpected, since this is never mentioned in the docs (or I did not see it)
this upgrade resulted from running npx ng update as recommended in the upgrade guide. Strange enough - it also recommended to upagrade the cli to version 13 - but I think this should/ will be done later in the guide.
ng-bootstrap required angular 12 - and prevented the migration to angular 13
ng-select@6 defines a peer dependency to angular 12 and needs to be upgraded before upgrading to angular 13
this is the result of running the migration script from https://update.angular.io/?l=2&v=11.0-13.0

Compiling the componenent my-absences-abstract-confirm failed probably due to the new typescript version
tslint is not supported with angular 13 - move to eslint with a bare bones setup (only recommended styles + prettier)
linting on commit is disabled for now because its failing for now, but I want to commit the current step in the migration process
the new eslint setup discourages empty lifecycle methods and reports the following error: Lifecycle methods should not be empty  @angular-eslint/no-empty-lifecycle-method
eslint complains about negated async pipes with the following error:  Async pipe results should not be negated...
the rule is:  @angular-eslint/template/no-negated-async
you should always use === or !== to avoid type coersion in javascript/ typescript. Enforced by the eslint rule @angular-eslint/template/eqeqeq
eslint reports this method as an issue, but angular throws errors in the console if this method is missing.
This error is then reported in the application - ignore the rule for this line
the eslint rule enforced strict comparisons - but the tests did not properly setup the loading observable
ng-bootstrap requires bootstrap 4
@mburri mburri requested a review from caebr February 9, 2022 10:57
Copy link
Collaborator

@caebr caebr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ich fand die ausführlichen Commit Messages recht hilfreich

@mburri mburri merged commit fa8e8e2 into master Feb 9, 2022
@mfehlmann mfehlmann deleted the 350-framework-lifecycle branch October 17, 2022 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants