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

RFC: Add HttpOptions to EntityActionOptions to allow finer control over HttpClient requests #3663

Closed
1 of 2 tasks
cam-m opened this issue Nov 15, 2022 · 5 comments · Fixed by #3664
Closed
1 of 2 tasks

Comments

@cam-m
Copy link
Contributor

cam-m commented Nov 15, 2022

Which @ngrx/* package(s) are relevant/related to the feature request?

ngrx/data

Information

Currently it is difficult to influence http requests sent in response to dispatched EntityActions. Simple requirements such as adding query parameters or headers require multiple ngrx/data classes to be overriden and provided. Often those implementing ngrx/data have an existing api they need to integrate with, and endpoints that require more than just an id/entity/list of entities to function correctly.

Proposing to add HttpOptions to EntityActionOptions that support finer control over the http requests executed by Data Services.

Change summary:

  • New EntityActionOptions property: HttpOptions:
/** Options of an EntityAction */
export interface EntityActionOptions {

  /** Correlate related EntityActions, particularly related saves. Must be serializable. */
  readonly correlationId?: any;

  /** other properties hidden for brevity */

  /** Options that will be passed to the dataService http request. Allows setting of Query Parameters and Headers */
  readonly httpOptions?: HttpOptions;
}

/**
 * Options that adhere to the constructor arguments for HttpParams and
 * HttpHeaders.
 */
export interface HttpOptions {
   httpParams?: HttpParamsOptions,
   httpHeaders?: string | {[p: string]: string | string[]};
}
  • EntityEffects to be updated to pass HttpOptions from an EntityAction to DataService requests
  • EntityCollectionDataService api to be updated to include optional HttpOptions in each supported request
  • DefaultDataService to be updated to pass HttpOptions along to the HttpClient requests. Both httpParams and httpHeaders are the correct type for the constructors of their respective angular classes: HttpParams and HttpHeaders as at Ng14.
  • getWithQuery should continue to accept QueryParams in arg 1 so as not to break clients. HttpOptions includes passing Http Params and is available in arg 2.
    If specified, http params in HttpOptions will replace those in arg 1 QueryParams. QueryParams has been annotated as deprecated, however removing the first arg would be a breaking change so looking for guidance from the core team on whether deprecation is required here. It might be possible to retire getWithQuery and use getAll with HttpOptions instead. Food for thought.
  • QueryParams type has been updated to reflect the (now exported) HttpParamsOptions fromObject type def (adds boolean, number, and their respective ReadOnlyArray types).

Related Issues

This issue proposed passing the entire EntityAction to data services. This comment by @wardbell outlines the solution.
I've taken a different approach for a couple of reasons.

  1. This solution provides a clear api to query params in the Data Service requests from data passed into the action. Passing EntityAction would make data available, but it would still require the user to register a custom data services in order to make use of the extra data to form query params or manipulate the url.
  2. I'm erring on the side of not 'oversharing' - provide everything in case its useful. Instead I provide data that directly integrates into the http request so provides a clear way to add query parameters and an encoder, and/or headers. I've currently I also forsee needs arising for using the HttpContext feature, and possibly (for the sake of requirements that can't be anticipated) a userDefinedOptions: any property to HttpOptions so user's can pass whatever they want.

Describe any alternatives/workarounds you're currently using

The workaround to this would be having to provide custom implementations of various ngrx data classes and factories. This ability also offers an alternative to hijacking the EntityAction tag property.

I would be willing to submit a PR to fix this issue

  • Yes
  • No
@cam-m cam-m changed the title Add HttpOptions to EntityActions to allow finer control over HttpClient requests Add HttpOptions to EntityActionOptions to allow finer control over HttpClient requests Nov 15, 2022
cam-m pushed a commit to cam-m/ngrx-platform that referenced this issue Nov 15, 2022
cam-m pushed a commit to cam-m/ngrx-platform that referenced this issue Nov 15, 2022
@cam-m cam-m changed the title Add HttpOptions to EntityActionOptions to allow finer control over HttpClient requests RFC: Add HttpOptions to EntityActionOptions to allow finer control over HttpClient requests Nov 15, 2022
@smorandi
Copy link

Honestly, while i get your point of not oversharing information, my original usecase was to be able to access data from url‘s which deviate from the current pattern required by ngrx data in certain conditions, e.g. accessing subressources which do not work with query-parameters. so thats why passing in the action itself (which can be customized, i.e. extended to carry additional data as of how to retrieve data) would be a better solution in my case.

so in my use-case is actually not about adding headers or query-parameters. but to actually alter the whole url which is otherwise not possible with a simple httpcustomizer curently provided.

but i know it‘s hard to find a balance flexibility and clear cut apis. i just think passing in the action is probably still better in my eyes.

@smorandi
Copy link

smorandi commented Nov 16, 2022

i see that #3093 deals with the exact use-case i was mentioning. yeah that‘s the issue i thought could be resolved by passing in a customized action into the data-service…

so yes, sorry for interfering with your rfc.

@cam-m
Copy link
Contributor Author

cam-m commented Nov 16, 2022

Don't be @smorandi, thanks for taking the time to comment :)

I'm in the process of migrating to ngrx/data and I have similar endpoints I will have to deal with, like /parent-resources/:id/related-resources.

As I'm in control of these endpoints I'll probably use routes like /related-resources/?parent-resource-id=:id , which is still semantically correct but fit better with ngrx/data's 'normalized' nature and single-Data-Service-per-entity approach.

Not much help if you can't change your api though.

Although, if this RFC goes ahead as is I think you'd be able to use the HttpOptions mechanism I've proposed to do what you want.

For example, you need to overrided one of the EntityCollectionDataService methods or the DataService.execute() method to change the url the way you want. You'd have the opportunity to make use of the HttpOptions I've added to the api...

E.g.

// entityCollectionDataService call
someService.getById(1, { httpParams: { fromObject: { children: 'related-resources' } } }

// updated data service method
getById(key: number | string, options?: HttpOptions): Observable<T> {
    let err: Error | undefined;
    if (key == null) {
      err = new Error(`No "${this.entityName}" key to get`);
    }
    if (options?.httpParams?.fromObject?.children) {
        return this.execute('GET', this.entityUrl + key + '/' + options?.httpParams?.fromObject?.children, err);
    }
    return this.execute('GET', this.entityUrl + key, err, options);
  }

I think there's still other work needed to ensure the reducers handle success correctly, but does this get you close to what you want?

@smorandi
Copy link

yes sure it would be fine, as long as i can pass „arbitrary“ data into the data service it would be absolutely fine. using httpoptions -> httpparams as the container for additional information works certainly well enough.

cam-m pushed a commit to cam-m/ngrx-platform that referenced this issue Jan 16, 2023
…trol over HttpClient requests (ngrx#3663)

added doco - rewrite to be non-breaking
cam-m pushed a commit to cam-m/ngrx-platform that referenced this issue Feb 1, 2023
…trol over HttpClient requests (ngrx#3663)

 - Removed redundant property of HttpOptions
 - Added warnings re deprecation of queryParams in getWithQuery
 - make HttpOptions type serializable
cam-m pushed a commit to cam-m/ngrx-platform that referenced this issue Feb 1, 2023
…trol over HttpClient requests (ngrx#3663)

 - fixed warnig message and made it coniitional
@e-oz
Copy link
Contributor

e-oz commented Feb 19, 2023

Please mention breaking changes like this in CHANGELOG.md

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

Successfully merging a pull request may close this issue.

4 participants