Skip to content

Feature/angular16 compatibility update - #53

Merged
jeanettephung merged 24 commits into
zingchart:masterfrom
Bramikke:feature/angular16-compatibility-update
Sep 6, 2023
Merged

Feature/angular16 compatibility update#53
jeanettephung merged 24 commits into
zingchart:masterfrom
Bramikke:feature/angular16-compatibility-update

Conversation

@Bramikke

@Bramikke Bramikke commented Aug 27, 2023

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
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

@Bramikke Bramikke left a comment

Copy link
Copy Markdown
Contributor Author

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.

@Bramikke Bramikke left a comment

Copy link
Copy Markdown
Contributor Author

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?

Comment thread package.json Outdated
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