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

feat(notification-center-angular): Support Angular versions 15+ #4518

Merged
merged 28 commits into from
Oct 25, 2023

Conversation

rifont
Copy link
Contributor

@rifont rifont commented Oct 13, 2023

What change does this PR introduce?

  • Moves Angular Notification Center dependencies from the Angular Workspace root to the library project
  • Allows any Angular version >=15 to be used with the NC by specifying loose @angular/* peer dependencies
  • Adds @types/react as a dependency to eliminate a step for Angular clients. This isn't ideal, but neither is shipping a full React dep in an Angular project. This is a viable DX improvement until we support Angular natively.

Why was this change needed?

  • The Angular NC only supported version 15 with the previous implementation. Angular requires singleton @angular/* dependency to work correctly, so we fix this by specifying these Angular dependencies as peer

Closes #3988.

Other information (Screenshots)

Successfully verified to be working with Angular 15, 16, & prerelease of 17 via a test NPM package (https://www.npmjs.com/package/rifont-notification-center-angular)

V15 (Currently LTS, EOL in 3 weeks)
angular-15-verification

V16 (Currently LTS)
angular-16-verification

V17 (tested with pre-release, GA is in 3 weeks)
angular-17-verification

Package Publish dry run
image

Lerna version bump testing
image

image

ReadMe Update
image

@linear
Copy link

linear bot commented Oct 13, 2023

NV-1919 The `notification-center-angular` package is not working with the latest Angular CLI generated project

Reproduction Steps

Use the Angular CLI to generate the new project, then install the notification-center-angular package and follow the instructions from the documentation: https://docs.novu.co/notification-center/client/angular.

At the end you won't see the Notification Center component in the app, and there will be error in the console.

Screenshot 2023-03-28 at 10.39.38.png

Also the example app in the main repo is not working as well.

Expected Behaviour

The Notification Center component should be rendered and should work well.

@rifont rifont changed the title Nv 1919 notification center angular fix: Oct 13, 2023
@rifont rifont changed the title fix: fix(notification-center-angular): Refactor dependencies to publish angular NC correctly Oct 13, 2023
@rifont rifont changed the title fix(notification-center-angular): Refactor dependencies to publish angular NC correctly feat(notification-center-angular): Support all Angular versions >=15 Oct 13, 2023
@rifont rifont marked this pull request as ready for review October 13, 2023 16:35
@rifont rifont changed the title feat(notification-center-angular): Support all Angular versions >=15 feat(notification-center-angular): Support Angular versions 15+ Oct 16, 2023
package.json Show resolved Hide resolved
pnpm-workspace.yaml Show resolved Hide resolved
@rifont
Copy link
Contributor Author

rifont commented Oct 17, 2023

@scopsy @LetItRock this PR has been updated to keep the current lerna publish workflow working uninterrupted. It is ready for review.

Copy link
Contributor

@LetItRock LetItRock left a comment

Choose a reason for hiding this comment

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

really awesome work @rifont! 🔥 let's polish it a little and merge! 🚀

@LetItRock
Copy link
Contributor

@jainpawan21 @unicodeveloper fyi

@rifont rifont self-assigned this Oct 24, 2023
@rifont rifont merged commit 79793bb into next Oct 25, 2023
26 of 27 checks passed
@rifont rifont deleted the nv-1919-notification-center-angular branch October 25, 2023 09:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐛 Bug Report: Error integrating @novu/notification-center-angular
3 participants