Update to Angular 9 #262
Merged
Update to Angular 9 #262
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Owner
madoar
commented
Feb 8, 2020
•
madoar
commented
madoar
commented
Feb 8, 2020
There was a problem hiding this comment.
I think this PR is ready to be merged. I've added some comments to some in my opinion important or interesting changes. If possible I would like for @earshinov to take a look at this PR to see whether he can find anything that I've missed.
If everything is ok I think we can merge this PR. Afterwards I would like to do some followup changes that are possible because of the newer typescript version we use now. Foremost I want to introduce Optional Chaining and Nullish Coalescing to make some routines more compact and maybe also easier to understand.
| @ViewChild(WizardComponent, {static: false}) | ||
| @ViewChild(WizardComponent) |
There was a problem hiding this comment.
This is an automatic change by angular-cli
| [ngStyle]="{ 'font-family': step.stepSymbolTemplate ? '' : step.navigationSymbol.fontFamily }"> | ||
| [style]="{ 'font-family': step.stepSymbolTemplate ? '' : step.navigationSymbol.fontFamily }"> |
There was a problem hiding this comment.
Angular 9 allows the use of the style and class bindings in addition to the "legacy" ngStyle and ngClass directives. See also section "Improved CSS class and style binding" in https://blog.angular.io/version-9-of-angular-now-available-project-ivy-has-arrived-23c97b63cfa3
Comment on lines
-478
to
+472
| expect(navBarEl.classes).toEqual({ | ||
| 'horizontal': true, 'vertical': false, 'small': false, | ||
| 'large-filled': true, 'large-filled-symbols': false, 'large-empty': false, 'large-empty-symbols': false | ||
| }); | ||
| expect(navBarEl.classes).toEqual({ 'horizontal': true, 'large-filled': true }); |
There was a problem hiding this comment.
It seems like the contract of classes has changed. The returned object now only contains the active classes, i.e. all classes that are true. This change affected 11 tests, which were "fixed" by me.
tsconfig.lib.prod.json
Outdated
| { | ||
| "extends": "./tsconfig.lib.json", | ||
| "angularCompilerOptions": { | ||
| "enableIvy": false |
There was a problem hiding this comment.
This file has been generated by angular-cli. Most likely enableIvy should continue to be false because of here. @earshinov what do you think? Should we change the value to true?
There was a problem hiding this comment.
You are right about using false. It needs to be false to preserve backward compatibility of angular-archwizard library with applications still using ViewEngine.
|
Looks good to me. I think it can be merged (save the |
|
Ok I just tried to build the demo repository against this branch. Long story short it does not seem to work. The initial problem was that typescript 3.7 contains some breaking changes compared to typescript 3.5. This also answers the question whether an Angular 9 library is compatible with an Angular 8 (or earlier) application. The answer seems to be no. After trying to update the demo repository to Typescript ng update @angular/core @angular/cli --allow-dirtyI got the following errors when calling After a bit of additional search I found out that these errors could be prevented by deactivating Ivy and aot. After deactivating Ivy and aot I'm a bit clueless how to continue. @earshinov do you by chance have an idea? |
|
Hmm, there shouldn't be so much trouble. I will try to take a look tomorrow. |
earshinov
added a commit
to earshinov/angular-archwizard
that referenced
this pull request
Feb 15, 2020
|
@madoar , you might have noticed that the demo project does not recognize the Angular migration script applies it automatically to the base I have pushed changes I had to make to
Also I would recommend to always build the library with |
|
@earshinov thank you for investigating this! I've applied your commits to my branches.
I'm not sure which other configuration files I need to modify, and how to modify them, to make this change work. Can you by chance do the necessary changes and I apply them afterwards on my branch? |
|
Sure. Please see this commit: earshinov@e767542 |
@earshinov thanks again for your help! I've just pushed some additional changes/improvements. More specific I have:
@earshinov could you please take another look at this PR and see if I have missed anything? If everything is all right I think we can merge this. |
|
I forgot to mention that it would also be nice if we could update the remaining packages that are kind of independent of Angular as well, e.g. |
|
The changes look good to me. As for scss-bundle, it can be upgraded at a later date. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.