Skip to content

Feature/angular16 compatibility update #53

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

Merged

Conversation

Bramikke
Copy link
Contributor

@Bramikke Bramikke commented Aug 27, 2023

Upgrade Zingchart-angular to be compatible with future Angular versions

This pull request fixes issues with Angular 16 and up compatibility, introduces the migration from TSlint to ESlint, and fixes issues preventing compilation and workflow runs. The following changes have been made:

  • Upgrade TSlint to ESlint
  • Addressed linting issues, including the conversion of 'let' to 'const', resolved missing imports, and ensured all components are suffixed with "component" for consistency.
  • Preserved original component names to facilitate a seamless transition for older projects.
  • Updated peer dependencies to the latest version of Angular, ensuring compatibility with forthcoming Angular releases.
  • Removed the previously unused 'zingchart-angular.service.ts' service from the library.
  • Corrected Zingchart typings to ensure proper compilation to the output directory.
  • Adjusted relevant tests to achieve full test suite pass.

These changes collectively enhance code quality, maintainability, and future compatibility. Please review the modifications and provide your feedback.

Note: All files are formatted by Prettier.

@agois83 agois83 requested a review from jeanettephung August 28, 2023 16:46
This file only deploys the app and not the library.
Building the library is redundant here.
`--base-href` flag needs to be set to the repo name since the Github Pages repo URL is "https://{userid}.github.io/{reponame}"
@Bramikke
Copy link
Contributor Author

@jeanettephung, building the library before building the application in deploy.yaml is required to successfully build the app.
The app is using the library. if the library is not created yet, the app build will fail.

Test using Chromeheadless to avoid Github actions being stuck during testing.
@Bramikke
Copy link
Contributor Author

I commited a small fix to ensure Github Actions will correctly run test.yaml because Github requires Chromeheadless.

Demo app requires the zingchart-angular dependency.
Without this, the "deploy" github action will build app without this dependency and not render the charts.

The `src/app/` components make references to this dependencies.
Support versions 16.2.0 to < 17
Support versions 16.2.0 to < 17
Remove comma I accidentally added
Update to match changes in package.json
Copy link
Contributor Author

@Bramikke Bramikke left a comment

Choose a reason for hiding this comment

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

projects/zingchart-angular/package.json

Please leave this on "latest" as setting a version here will prevent compatibility with future Angular versions.
By setting this, you oblige the user to install it's packages using --legacy-peer-deps. This is not ideal because it is possibly breaking other packages.

Copy link
Contributor Author

@Bramikke Bramikke left a comment

Choose a reason for hiding this comment

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

package.json

Downgrading these versions does not make any sense.
This is a stand alone application which cannot be reused elsewhere. Why would you downgrade as the library is fully compatible with the newer versions?

Reverting my changes.
I see that the build fails and the app is now using the library instead of the library from the node modules directory.
Reverting my commit
Reverting since the app now uses the library instead of the npm package
Update to match package.json revert
Support angular 16.2.0 +
@jeanettephung jeanettephung merged commit c36b7be into zingchart:master Sep 6, 2023
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.

2 participants