dalelotts / angular-bootstrap-datetimepicker

Native Angular date/time picker component styled by Twitter Bootstrap
http://dalelotts.github.io/angular-bootstrap-datetimepicker/
MIT License
1.28k stars 409 forks source link

Feature/angular update to v13 #515

Closed tszewcow closed 7 months ago

tszewcow commented 8 months ago

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...) The PR introduces the update of the core angular dependencies in the library repo from angular v8.x.x to angular v.13.x.x. The reason of this change is that with the previous setup the lib is built with so-called legacy "view engine" compiler. When this is the case and someone wants to use the lib in the angular app starting from v16, the code won't compile as the view-engine compiler is not longer shipped with the angular v16 (and newer versions). The lib has to be published as an Ivy distribution.

If you install the lib (current version 4.0.2) in e.g. angular v15 app and you trigger the ng --serve command you will get the following compilation warning: "Encourage the library authors to publish an Ivy distribution" Doing the same in angular v16 will end up with compilation error and the app won't start with the angular-bootstrap-datetimepicker lib installed as a dependency.

What is the current behavior? (You can also link to an open issue here) The library is not installable in the projects built on top of Angular 16 and newer. Issue: https://github.com/dalelotts/angular-bootstrap-datetimepicker/issues/486

What is the new behavior (if this is a feature change)? The app works as before and it is additionally installable in the newest angular apps (tested on angular projects where base app uses ng v15 and ng v17). Here the example of the pure ng-17 app with the date-time picker: datepicker_ng_17

Does this PR introduce a breaking change? Yes, the lib core deps were bumped and I recommend to install the lib in the angular apps starting from v13 and newer. I strongly recommend to publish the new version of the lib with the 13.0.0 version to match the used angular core deps.

Please check if the PR fulfills these requirements

Other information:

Next recommended steps / recommendations for feature releases:

dalelotts commented 8 months ago

Thank you for your contribution!!

Do the protractor tests no long run / work or is the deletion of those tests only because the project is past end-of-life?

tszewcow commented 8 months ago

Well... it's both. I struggled for hours to get them up to even start. The problem is that the webdriver-manager downloads the latest available chromium which is the version === 114 (https://github.com/bonigarcia/webdrivermanager/blob/master/src/main/resources/versions.properties) and my local chrome is already on v120. If there is a version mismatch the tests won't run at all. I've hacked it so I was able to start the tests but they couldn't locate the change year|month|day|hour button of the datepicker (although it is there). I tried to start the tests in debug mode - unfortunately without a success, so I decided to get rid of them. If you think I should bring them back but remove them from "test" script then I will do it.

dalelotts commented 8 months ago

That's very helpful, thank you!

I'd prefer to have running tests for the functionality, but if they don't work something has to be done.

Do you have a sense of the effort to port the tests to something else that is still supported?

tszewcow commented 8 months ago

Yeah, i recommend to switch to cypress e2e framework. It’s quite simple to write end to end tests in it. Even the newest angular cli lists it as one of the options when calling ng e2e and there is schematics for it. I would open an additional issue for the case if this is not a problem. I don’t thing it will cost much to port your protractor tests to some other framework, there are not a lot of them. I guess it would be achievable in 1-2md

dalelotts commented 8 months ago

I understand. I'm just pulling this down locally for review. I have a few things to think about before merging - nothing about your changes, at first glance everything looks good - I'll let you know if I have any more questions.

dalelotts commented 8 months ago

Oh, I assume you are using this for a project, can you tell me a bit about your requirements? (I assume Cypress for e2e)

Angular version, node version, anything else I should know?

tszewcow commented 8 months ago

I was using cypress in my previous project which was built on top of react, we run the e2e test suite against the deployed app so the underlying technology didn't matter. Actually, I would say cypress is kind of "framework-agnostic" - you can test with it wathever web app you want so there are no specific angular version requirements to make it work. Though, you can integrate it with your angular tooling. Please see https://docs.cypress.io/guides/component-testing/angular/overview As far as I can see the tests in the examples are not ran via angular cli, but via this npm script: https://github.com/cypress-io/cypress-component-testing-apps/blob/a4f620d9b69d9165c9ce7a6bc8a5eb21729bf50b/angular-standalone/package.json#L10 I assume that the script is triggering the angular dev server with your app in dev mode and starts the e2e test run against this server.

dalelotts commented 8 months ago

Sorry, I think you misunderstood my question. I am wondering if you are using this component in a project and if so, what version of Angular, node, etc are you using.

I am asking because I am reluctant to release a new version of this component that is not fully tested - it would likely be easier for me to take your changes, upgrade to Angular 17 and use cypress for the e2e tests - that seems easier and faster than trying to get a bunch of unsupported packages to work well. Does that makes sense?

tszewcow commented 8 months ago

Ah, yes, sorry for misunderstanding. I am currently using the component in a project where we have angular v15.2.10. I am planning migration to Angular v16, that's why I need a Ivy distribution of it. Also I am about to install the component in another project based on Angular v17.0.8. In both projects I am working on node 18.18.2 Hydrogen LTS version.

Yes -> it makes sense to bump the angular version in this repo to angular 17 and use cypress as a replacement for the protractor when it's about e2e testing. Sorry for not doing this myself but this would be much more time consuming than bumping the ng version to v13. This pull request is kinda a "minimal" solution to make the component work in projects with angular v16/17 where there is no more legacy view-engine compiler.

dalelotts commented 8 months ago

I really appreciate all of your work on this! That's why I don't want to do anything that will mess up your project plans.

If I update to Angular 17, will that work for you? Or should I stop at 16?

tszewcow commented 8 months ago

No worries, thank you for maintaining this repo and trying to keep it up with the newest technologies.

It’s fully ok to use the newest angular (17). If the lib is built on top of it, I will still be able to install and use this component in angular 15/16/17 based projects.

Thx again!

tszewcow commented 8 months ago

Hey @dalelotts - FYI - I decided to port your protractor e2e tests to the cypress specs. Today I am planning to push two commits:

You can then decide what to do:

tszewcow commented 8 months ago

@dalelotts - it's done. I've ported your protractor tests into the cypress test-suite. You can run them via:

dalelotts commented 7 months ago

Sorry for the delay, I'll get to this ASAP.

dalelotts commented 7 months ago

I went ahead and merged without much review since overall it looked good and it will be a major version update, but not v13 since this project follows semver.org

tszewcow commented 7 months ago

Thanks! I guess you can now close this issue: https://github.com/dalelotts/angular-bootstrap-datetimepicker/issues/486

I will now patiently wait for the new package release here: https://www.npmjs.com/package/angular-bootstrap-datetimepicker Thx again.

dalelotts commented 7 months ago

it should have published automatically, but for some reason Travis-ci is no longer building this repo. I'll have to sort that out, hopefully this weekend.

dalelotts commented 7 months ago

Quick update on publishing - travis was setup to automatically release a new version to npm when certain changes happen via semantic-release - of course it stopped working at some point but I didn't notice because I didn't make any changes. The release analysis works just fine locally but not on travis. Still looking into it as time allows.

gmiller-in-situ commented 5 months ago

Thank you so much for your work on this! We're working on updating to Angular 16 and I've just copied this library in as a local nx lib for now. I'm looking forward to the package being released on NPM!

dalelotts commented 5 months ago

Thank you for the reminder - I will have time next week to fix the semantic release - my apologies to @tszewcow, et al, for the delay

dalelotts commented 4 months ago

Update: I decided to change from TravisCI to github actions and update to the latest semantic-release version - this meant removing karma totally - so I started the migrate-to-jest branch - there are a few tests that are failing due "Expected to be running in 'ProxyZone', but it was not found."

npm run test:ci is the target script that is failing - is anyone familiar with this failure and how to fix it?

vatsalkgor commented 1 month ago

Hi @dalelotts appreciate the work on this PR. Is there any chance you've made progress on releasing this update? Thanks.

dalelotts commented 1 month ago

I have not looked at it in some time, I am working on anther project that I hope to release in the next week.

I will release as soon as the CI tests are passing - I would appreciate any help in getting the tests to pass.