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 #188

Closed
neodescis opened this issue Nov 9, 2023 · 19 comments
Closed

Angular 17 #188

neodescis opened this issue Nov 9, 2023 · 19 comments

Comments

@neodescis
Copy link

neodescis commented Nov 9, 2023

It would be great to add support for Angular 17. I've already tested things by overriding the peer dependencies, and everything seems to work just fine, so I think we just need to update the peer dependencies and publish.

I would be happy to create a pull request, but I'm wondering... instead of just adding || ^17.0.0, can we specify >= 13.0.3?

@neodescis
Copy link
Author

Also, @danielmoncada, if you're looking for someone to help out on this repo, I'd be happy to take up a contributor or even maintainer role.

@polhek
Copy link

polhek commented Nov 19, 2023

@neodescis i was trying to make the library work locally to test if it works. But after "npm install" and "ng serve" in the root directory, I get quite a lot of errors. That is without any changes, just forking the repo and trying it out. Can you point me in the right direction, please.

Node version: v16.20.2

image

@danielmoncada
Copy link
Owner

@neodescis ah, didn't realize a new ng version was out. yeah, I can try and update it tonight, thanks for testing.

@neodescis
Copy link
Author

neodescis commented Nov 22, 2023

@neodescis i was trying to make the library work locally to test if it works. But after "npm install" and "ng serve" in the root directory, I get quite a lot of errors. That is without any changes, just forking the repo and trying it out. Can you point me in the right direction, please.

Node version: v16.20.2

image

When I say I got things working with Angular 17, I mean I just added an entry to "overrides" in my project's package.json to get past the peer dependency error with installing and running Angular 17 along side this package. I did not do what you attempted.

I will also add that if we update this package to be built with Angular 17, it will break compatibility with previous Angular versions, as built libraries are not backward compatible with earlier versions of Angular. Rather, my point was that updating the peer dependencies is good enough.

@polhek
Copy link

polhek commented Nov 22, 2023

@neodescis i was trying to make the library work locally to test if it works. But after "npm install" and "ng serve" in the root directory, I get quite a lot of errors. That is without any changes, just forking the repo and trying it out. Can you point me in the right direction, please.

Node version: v16.20.2

image

When I say I got things working with Angular 17, I mean I just added an entry to "overrides" in my package.json to get past the peer dependency error with installing and running Angular 17 along side this package. I did not do what you attempted.

Also, if we update this package to be built with Angular 17, it will break compatibility with previous Angular versions, as built libraries are not backward compatible with earlier versions of Angular. Rather, my point was that updating the peer dependencies is good enough.

I forked the repo, then 'npm install' and then ng serve or one of the build the library, but none of it worked. Resulting in the errors in the screenshot.

@neodescis
Copy link
Author

I forked the repo, then 'npm install' and then ng serve or one of the build the library, but none of it worked. Resulting in the errors in the screenshot.

Yes, I understand. I ran npm install, npm run build_lib and npm start and things are working just fine for me. I used node 16.19.0 because I already had it installed via nvm.

@pavlikxor
Copy link

pavlikxor commented Nov 24, 2023

Hi Everyone
@danielmoncada
Please check the PR
Added Angular 17 support
#189

@neodescis
Copy link
Author

neodescis commented Nov 24, 2023

@pavlikxor, as I mentioned above, if we switch to actually building with Angular 17, it is no longer backward compatible with earlier versions of Angular. Is that what we want to do?

@neodescis
Copy link
Author

Also, this is not the only repository that needs updating. There are separate repos for moment and Day.js adapters.

@pavlikxor
Copy link

@pavlikxor, as I mentioned above, if we switch to actually building with Angular 17, it is no longer backward compatible with earlier versions of Angular. Is that what we want to do?

Correct,
I see no reasons keeping it backward compatible. For apps developed using older Angular versions (<= 16) appropriate library version should be used

@pavlikxor
Copy link

Also, this is not the only repository that needs updating. There are separate repos for moment and Day.js adapters.

Correct,
I'll take care of adapter-libraries update as well. But firstly main library needs to be updated, as adapter-libraries have devDependency on @danielmoncada/angular-datetime-picker

@NetWin
Copy link

NetWin commented Dec 1, 2023

What's the current state here? Is there an ETA for the release? 😄
@danielmoncada @neodescis @pavlikxor

@neodescis
Copy link
Author

I believe there are two things outstanding in the code review that @danielmoncada had thumbs-up'd:

  1. The change to disableClose in dialog-ref.class.ts
  2. Changing the peer dependencies to be >= 17.0.0

@danielmoncada
Copy link
Owner

I have some time now.. pulling the PR now

@danielmoncada
Copy link
Owner

alright; latest has been published to npm. still need to update the adapters.

@pavlikxor
Copy link

pavlikxor commented Dec 4, 2023

Hi everyone
cc @danielmoncada
Moment adapter, please review - danielmoncada/date-time-picker-moment-adapter#15
Dayjs adapter - danielmoncada/date-time-picker-dayjs-adapter#15

@neodescis
Copy link
Author

alright; latest has been published to npm. still need to update the adapters.

I guess we're not going with >=17.0.0 in the peer dependencies then? Bummer.

@neodescis
Copy link
Author

Hi everyone cc @danielmoncada Moment adapter, please review - danielmoncada/date-time-picker-moment-adapter#15 Dayjs adapter - danielmoncada/date-time-picker-dayjs-adapter#15

I added a few comments to the dayjs adapter. The moment adapter seems like it should have similar tsconfig changes to what you did for dayjs though, no?

@danielmoncada
Copy link
Owner

closing, both adapters and the picker have now been updated (and published to npm).

thank you for the help / work @pavlikxor and @neodescis

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

No branches or pull requests

5 participants