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(data): Add HttpOptions to EntityActionOptions (#3663) #3664

Merged
merged 12 commits into from
Feb 9, 2023

Conversation

cam-m
Copy link
Contributor

@cam-m cam-m commented Nov 15, 2022

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

Angular HttpParams and HttpHeaders cannot be passed from EntityCollectionDataService to underlying Data Service requests.

Closes #3663

What is the new behavior?

Http Params and HttpHeaders can be passed from EntityCollectionDataService to underlying Data Service requests.

Does this PR introduce a breaking change?

[] Yes
[x] No

MethodgetWithQuery(queryParams) can be refactored in a future change to remove the queryParams arg if desired. It is made redundant by the new httpOptions arg.

Other information

@netlify
Copy link

netlify bot commented Nov 15, 2022

Deploy Preview for ngrx-io ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit a6aa56f
🔍 Latest deploy log https://app.netlify.com/sites/ngrx-io/deploys/63da591b28269c00081fd022
😎 Deploy Preview https://deploy-preview-3664--ngrx-io.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@cam-m
Copy link
Contributor Author

cam-m commented Nov 15, 2022

This is my first ever pull request so please let me know if I've stuffed anything up :)

@brandonroberts
Copy link
Member

As this would be a breaking change, it would have to wait until the next major release. Also, does this PR #3717 achieve the same result?

@cam-m
Copy link
Contributor Author

cam-m commented Dec 30, 2022

As this would be a breaking change, it would have to wait until the next major release. Also, does this PR #3717 achieve the same result?

No, and I was able to work around #3717 using the clearCache method.

This PR provides more general capability - by adding httpoptions to entity actions users can pass query params to any http requests flowing from entity actions - I raised this so I could pass params to a delete request.

I considered doing it as a non-breaking change, but I think the api would be less coherent that way. If you think users would be taking advantage of the current options parameter then perhaps I should re-do as non breaking...?

@cam-m
Copy link
Contributor Author

cam-m commented Dec 30, 2022

Also if I'm not mistaken, this PR would solve #3717 by allowing the user to pass query params to the load action.

@brandonroberts
Copy link
Member

If there is an option to do a non-breaking change in the short term that would be preferable. We can deprecate the current usage and do a breaking change in the next major if necessary.

Cameron Maunder and others added 2 commits January 16, 2023 16:45
cam-m and others added 2 commits January 30, 2023 15:25
…trol over HttpClient requests (ngrx#3663)

 - Removed redundant property of HttpOptions
 - Added warnings re deprecation of queryParams in getWithQuery
 - make HttpOptions type serializable
…trol over HttpClient requests (ngrx#3663)

 - fixed warnig message and made it coniitional
@cam-m
Copy link
Contributor Author

cam-m commented Feb 1, 2023

As this would be a breaking change, it would have to wait until the next major release. Also, does this PR #3717 achieve the same result?

Hey @brandonroberts, just FYI this now a non-breaking change.

@brandonroberts brandonroberts merged commit dd745c0 into ngrx:master Feb 9, 2023
@brandonroberts
Copy link
Member

Thanks @cam-m!

@e-oz
Copy link
Contributor

e-oz commented Feb 19, 2023

It is a breaking change.
In 15.2.1 call execute('POST', urlWithParams, requestData, {observe: 'response'}) returns HttpResponse object (because of observe: 'response'), in 15.3 the same code returns just response body.

@timdeschryver
Copy link
Member

Do you want to create a Pull Request for this @e-oz ?

@e-oz
Copy link
Contributor

e-oz commented Feb 20, 2023

Pull Request to change what exactly? I don't mind :)

@cam-m
Copy link
Contributor Author

cam-m commented Feb 20, 2023

You're right @e-oz , I missed the case of user's adding http request options other than headers and params.

I'm happy to submit a PR to address this.

@brandonroberts
Copy link
Member

@cam-m that would be great thanks

@cam-m
Copy link
Contributor Author

cam-m commented Feb 28, 2023

FYI I've not forgotten about this, have had trouble getting the unit tests to run on my win machine - switched to a mac and should have a PR in today.

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.

RFC: Add HttpOptions to EntityActionOptions to allow finer control over HttpClient requests
4 participants