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

Angular 17 support #189

Merged
merged 17 commits into from
Dec 4, 2023
Merged

Conversation

pavlikxor
Copy link

@pavlikxor pavlikxor commented Nov 24, 2023

Angular v.17 support added

@pavlikxor pavlikxor mentioned this pull request Nov 24, 2023
"@angular/platform-browser": "^13.3.5",
"@angular/platform-browser-dynamic": "^13.3.5",
"@angular/router": "^13.3.5",
"@angular/animations": "^17.0.4",

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!

Copy link
Author

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

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...

Copy link
Author

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;

Choose a reason for hiding this comment

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

Why this change?

Copy link
Author

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

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!

"@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",
Copy link

@neodescis neodescis Nov 24, 2023

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

Copy link
Author

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

@@ -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'

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

Copy link
Author

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",

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.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, in deed

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",

Choose a reason for hiding this comment

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

Suggested change
"@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",

Choose a reason for hiding this comment

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

Suggested change
"@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"

Choose a reason for hiding this comment

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

Suggested change
"@angular/cdk": "^17.0.0"
"@angular/cdk": ">=17.0.0"

@danielmoncada
Copy link
Owner

fixes #188

@danielmoncada danielmoncada merged commit 1743db8 into danielmoncada:master Dec 4, 2023
1 check passed
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.

3 participants