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

Adds options parameter #101

Closed
wants to merge 5 commits into from
Closed

Adds options parameter #101

wants to merge 5 commits into from

Conversation

lizell
Copy link
Contributor

@lizell lizell commented Sep 15, 2016

Adds options parameter to the CALL_API which is passed to the underlying fetch call. This allows us to control things like network timeouts and redirects, etc.

Example:

{
  [CALL_API]: {
    ...
    options: { timeout: 3000 }
    ...
  }
}

This is somewhat different than #79 that adds the possible options directly on the CALL_API.

@coveralls
Copy link

coveralls commented Sep 15, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 5bb43eb on Aftonbladet:feature/options into e72ac27 on agraboso:master.

@matthisk
Copy link

any reason this was never merged? This is a useful feature.

@nason nason added this to the 2.0.0 milestone May 1, 2017
@nason
Copy link
Collaborator

nason commented May 1, 2017

Thanks for the contribution @lizell. This project was unmaintained for a little not not anymore! Are you still interested in helping with this?

I've added this to the 2.0 milestone for now. I'm thinking it makes sense to to put this in the next branch / 2.0 release. What are your thoughts?

@lizell
Copy link
Contributor Author

lizell commented May 2, 2017

Wow! That is great news. Yes, we are already using this in production, so the sooner the better. ;) I'll look into the merge conflicts.

@agraboso
Copy link
Owner

agraboso commented May 2, 2017

@lizell Since this PR has been added to the 2.0.0 milestone, you should rebase it on the next branch.

A few comments:

  1. Note that isomorphic-fetch has been removed on the next branch in favor of a global fetch. Thus all references to isomorphic-fetch should also be removed.

  2. I also see that in your description of [CALL_API].options (which on next should be [RSAA].options) you specifically mention node-fetch. That should be replaced with a link to the fetch specification or maybe a comment that this object is intended as the options object of whatever fetch implementation is used — together with some examples of common options.

  3. Most critically: [RSAA].method, [RSAA].headers, [RSAA].body and [RSAA].credentials do belong in the options object. Moving them in there should be easy — though maybe a bit laborious, since much of the documentation would need to be changed.

    The last three are currently optional, but [RSAA].method is required. There are two possibilities that come to mind:

    1. Move [RSAA].method inside the options object, and default to GET if it is not given (so as not to make [CALL_API].options mandatory)
    2. Keep [RSAA].method as is, and merge it with the [RSAA].options before passing it to fetch.

    I somewhat lean in favor of the first one. What are your thoughts, @lizell and @nason?

  4. At the moment, [RSAA].headers may be a function (returning an object containing the actual headers). If we want to keep this possibility, we would need to accept as [RSAA].options either an object or a function returning such an options object.

@nason
Copy link
Collaborator

nason commented May 17, 2017

I went ahead and rebased this branch off of next and got everything synced up. That branch is available here: next...next-options-param

@lizell are you interested in finishing up the work for this feature? If so feel free to use that rebased branch! I could also help, but it won't be for a few days.

@agraboso:

  • I went ahead and cleaned up (1) and some of (2)
  • I think (4) is done already in this branch, unless I'm misunderstanding something
  • (3) is quite interesting. Moving these params into the options object makes sense, but does that need to be done for 2.0/this PR? It looks the 2nd option is implemented in this PR already and doesn't require any API changes.

I like the first approach more philosophically, but the 2nd seems more practical. Is there any reason not to go with it for now?

@gaultierq
Copy link

any update ? would be great to be able to configure timeouts

@lizell
Copy link
Contributor Author

lizell commented Sep 20, 2017

So… I'm sorry this took so long, but I have updated the docs, merged changes and done some cleanup and I am ready to push my code.

How would you like me to proceed? I don't have write access to next-options-param. Should I fork again and make a new PR to that branch or what do you think?

I left 3 and 4 untouched, since I think the API changes could wait and be done separately and 4 was apparently already done.

@lizell
Copy link
Contributor Author

lizell commented Sep 21, 2017

I went ahead and created a PR with my changes on top of yours @nason. You'll find it here: #150.

@nason
Copy link
Collaborator

nason commented Sep 21, 2017

This change is going to go out with the 2.0 release and was merged in #150 -- closing this PR.

@nason nason closed this Sep 21, 2017
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.

None yet

6 participants