-
Notifications
You must be signed in to change notification settings - Fork 60
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
Angular 17 support #189
Angular 17 support #189
Conversation
"@angular/platform-browser": "^13.3.5", | ||
"@angular/platform-browser-dynamic": "^13.3.5", | ||
"@angular/router": "^13.3.5", | ||
"@angular/animations": "^17.0.4", |
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.
Are we sure we want to do this? The library will not be backward compatible if we are building with Angular 17!
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.
I see no reasons to keep library backward compatible. If there is a need to use it in apps with Angular versions <= 16 - use appropriate library version
.travis.yml
Outdated
@@ -7,7 +7,7 @@ cache: | |||
addons: | |||
chrome: stable | |||
before_script: | |||
- npm install -g @angular/cli@9.0.4 | |||
- npm install -g @angular/cli@latest |
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.
This works until Angular introduces breaking changes to the cli package...
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.
Thanks, fixed version used
@@ -29,20 +29,21 @@ export class OwlDialogRef<T> { | |||
public componentInstance: T; | |||
|
|||
/** Whether the user is allowed to close the dialog. */ | |||
public disableClose = this.container.config.disableClose; | |||
public disableClose = 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.
Why this change?
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.
Getting TS2729: Property 'container' is used before its initialization
error while building
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.
Ah, I guess I missed that this is now set to this.container.config.disableClose
in the constructor instead. Looks good!
projects/picker/package.json
Outdated
"@angular/common": "^13.0.3 || ^14.0.0 || ^15.0.0 || ^16.0.0", | ||
"@angular/core": "^13.0.3 || ^14.0.0 || ^15.0.0 || ^16.0.0", | ||
"@angular/cdk": "^13.0.3 || ^14.0.0 || ^15.0.0 || ^16.0.0" | ||
"@angular/common": "^13.0.3 || ^14.0.0 || ^15.0.0 || ^16.0.0 || ^17.0.0", |
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.
IMO, we should just say >=13.0.3
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.
Corrected dependencies to support v.17 only
.github/workflows/ci_test.yml
Outdated
@@ -26,7 +26,7 @@ jobs: | |||
- name: Use Node.js | |||
uses: actions/setup-node@v1 | |||
with: | |||
node-version: '14.16.1' | |||
node-version: '16.16.0' |
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.
I thought Angular 17 required node 18
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.
Thanks, updated
"@angular/common": "^13.0.3 || ^14.0.0 || ^15.0.0 || ^16.0.0", | ||
"@angular/core": "^13.0.3 || ^14.0.0 || ^15.0.0 || ^16.0.0", | ||
"@angular/cdk": "^13.0.3 || ^14.0.0 || ^15.0.0 || ^16.0.0" | ||
"@angular/common": "^17.0.0", |
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.
Do we want to advertise as >=17.0.0
? Libraries are generally forward-compatible, and changing this to >=
would make things easier for consumers of the library going forward.
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, in deed
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.
So, I meant literally >=17.0.0
instead of ^17.0.0
This would allow the library to be used with Angular 18 when it's released, with no changes to the library.
"@angular/common": "^13.0.3 || ^14.0.0 || ^15.0.0 || ^16.0.0", | ||
"@angular/core": "^13.0.3 || ^14.0.0 || ^15.0.0 || ^16.0.0", | ||
"@angular/cdk": "^13.0.3 || ^14.0.0 || ^15.0.0 || ^16.0.0" | ||
"@angular/common": "^17.0.0", |
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.
"@angular/common": "^17.0.0", | |
"@angular/common": ">=17.0.0", |
"@angular/core": "^13.0.3 || ^14.0.0 || ^15.0.0 || ^16.0.0", | ||
"@angular/cdk": "^13.0.3 || ^14.0.0 || ^15.0.0 || ^16.0.0" | ||
"@angular/common": "^17.0.0", | ||
"@angular/core": "^17.0.0", |
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.
"@angular/core": "^17.0.0", | |
"@angular/core": ">=17.0.0", |
"@angular/cdk": "^13.0.3 || ^14.0.0 || ^15.0.0 || ^16.0.0" | ||
"@angular/common": "^17.0.0", | ||
"@angular/core": "^17.0.0", | ||
"@angular/cdk": "^17.0.0" |
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.
"@angular/cdk": "^17.0.0" | |
"@angular/cdk": ">=17.0.0" |
fixes #188 |
Angular v.17 support added