Skip to content

Sg set headers #77

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

Merged
merged 4 commits into from
Nov 24, 2019
Merged

Sg set headers #77

merged 4 commits into from
Nov 24, 2019

Conversation

badaz
Copy link
Contributor

@badaz badaz commented Nov 17, 2019

Possibility to add headers on a client's scope (up to now it was only possible by passing a param object to authorizedFetch, and not possible to pass it to methods like find, create, etc... in clients)

@badaz badaz requested a review from jdeniau November 17, 2019 16:01
@jdeniau
Copy link
Member

jdeniau commented Nov 18, 2019

I'm not really fan of setting the headers that will impact the all call of the repository instance.

One solution that may be better is to use a wither that will return a new instance of the Repository:

class AbstractClient {
  withHeaders(headers = {}) {
    const repository = cloneTheCurrentRepository(this);
    repository.headers = headers;

    return repository;
  }
}

but this is kind of hacky too.

An even better solution may be to refactor all the queryParam = {}, pathParameters = {} parameters to pass an object containing request-dependent variables, but it is more complicated to implement.
Maybe a "dirty" solution could be to add a last parameters headers = {} after those two to keep the same logic.

What do you think ?

@badaz
Copy link
Contributor Author

badaz commented Nov 18, 2019

I proposed a "dirty" solution already, dictated by lazyness I must admit, but if you prefer your "dirty" solution (aka adding a headers param) then I'll go for it ^^, just have to add the param everywhere. I do not think the wither is a lot better since there is no guarantee that one does not just declare client.headers = { someHeader: "foo" }. And a unique object with request dependent params would be breaking so I'll pass. What do YOU think ?

@jdeniau jdeniau mentioned this pull request Nov 18, 2019
@jdeniau
Copy link
Member

jdeniau commented Nov 18, 2019

Let's go for adding another parameter everywhere then, it would be much simpler to refactor this in a single object and the breaking changes will be easier to fix then.

I've opened #78 as a reminder

@badaz badaz merged commit 15846ad into master Nov 24, 2019
@jdeniau jdeniau deleted the sg-setHeaders branch November 25, 2019 20:34
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