Skip to content

feat: Enhanced withCallState to accept mutliple collection configuration #148

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Apr 1, 2025
Merged

feat: Enhanced withCallState to accept mutliple collection configuration #148

merged 8 commits into from
Apr 1, 2025

Conversation

@abhijit-chikane
Copy link
Contributor

@abhijit-chikane abhijit-chikane commented Feb 16, 2025 •
edited
Loading

feat: Enhanced withCallState to accept mutliple collection configuration
For #147
I have not tested it but happy to help if in case it didn't work as expected
thanks

@abhijit-chikane abhijit-chikane marked this pull request as draft February 16, 2025 11:08
@abhijit-chikane
Copy link
Contributor Author

abhijit-chikane commented Feb 16, 2025

This requires additional changes and need to fix the typescript errors

@abhijit-chikane abhijit-chikane force-pushed the main branch 4 times, most recently from c74218f to 86baae1 Compare February 26, 2025 11:04
@abhijit-chikane abhijit-chikane marked this pull request as ready for review February 26, 2025 11:05
@abhijit-chikane abhijit-chikane mentioned this pull request Feb 26, 2025
Closed
@rainerhahnekamp rainerhahnekamp added the ready-for-review This feature has been implemented and is ready for a review. label Mar 4, 2025
@abhijit-chikane
Copy link
Contributor Author

abhijit-chikane commented Mar 12, 2025

@rainerhahnekamp did you get time to review this?
Let me know if anything needs to be done from my side
Thanks!

rainerhahnekamp
rainerhahnekamp requested changes Mar 18, 2025
View reviewed changes
Copy link
Collaborator

@rainerhahnekamp rainerhahnekamp left a comment

Choose a reason for hiding this comment

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

Well done and sorry for the late reply. What I'm currently asking myself, if we could use collections instead of collection to support the array of strings?

Would that be a big issue?

@abhijit-chikane
Copy link
Contributor Author

abhijit-chikane commented Mar 18, 2025 •
edited
Loading

Well done and sorry for the late reply. What I'm currently asking myself, if we could use collections instead of collection to support the array of strings?

Would that be a big issue?

Well initially I did exactly that but then it would be a breaking change
That’s why I reverted it back to ‘collection’

@rainerhahnekamp
Copy link
Collaborator

rainerhahnekamp commented Mar 18, 2025

it would be a breaking change

In which regard? To me, it would be an additional property. It would be breaking if collections was available in the past and now got its behavior changed.

@abhijit-chikane
Update with-call-state.ts
f92a116 
fix(ngrx-toolkit): accept collection to collections in withCallState
@abhijit-chikane
Update with-call-state.spec.ts
b2b65b6 
fix unit test
@abhijit-chikane
Copy link
Contributor Author

abhijit-chikane commented Mar 27, 2025 •
edited
Loading

@rainerhahnekamp I have made the changes
Could you please review it?

one more thing is we no longer need getCallStateKeys method here
but as it is being used at other places in project so I have kept it there but we can mark it as deprecated

let me know if you want me to do that change or we should leave it as it is

Thanks

rainerhahnekamp
rainerhahnekamp approved these changes Apr 1, 2025
View reviewed changes
Copy link
Collaborator

@rainerhahnekamp rainerhahnekamp left a comment

Choose a reason for hiding this comment

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

LGTM

@rainerhahnekamp rainerhahnekamp merged commit 90a37ca into angular-architects:main Apr 1, 2025
1 check passed
@rainerhahnekamp
Copy link
Collaborator

rainerhahnekamp commented Apr 1, 2025

Thanks @abhijit-chikane

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rainerhahnekamp rainerhahnekamp rainerhahnekamp approved these changes

Assignees

No one assigned

Labels

ready-for-review This feature has been implemented and is ready for a review.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@abhijit-chikane @rainerhahnekamp