feat: add support for custom headers to send-request

[OR13]

This pull request adds a header object to the config, which can be used to provide default headers on all requests made by this library.

This is useful when IPFS is behind http proxies that require custom headers, such as Authorization.

[daviddias]

@OR13 thanks for adding this in. Mind adding a test and documenting this option?

[OR13]

I have added a test, and the build is passing in node 8, but not node 6.

https://travis-ci.org/ipfs/js-ipfs-api/jobs/370753539#L4269

[OR13]

@diasdavid is a passing node 6 build a requirement for this PR?

likewise, this test only covers the node case. is that acceptable?

[prettymuchbryce]

I think we may also need to add documentation, although I'm not completely sure where that lives. My best guess would be some mention in README.md.

[OR13]

@prettymuchbryce I added a short blurb under the CORS setup.

I misinterpreted the documentation request initially, sorry for the delay.

If you would like me to add additional detail about why custom headers are useful, I can do that.

I'm trying to keep this as simple as possible, as its not an option everyone will need.

[prettymuchbryce]

Your short blurb in the README makes sense to me. I think it's perfectly fine for now.

In terms of the tests, it looks like all of them are failing so I'm guessing it's not related to your change. It looks like Jenkins may be out of disk space. I'm not sure who's responsible for taking a look into that.

[OR13]

Jenkins is failing, but travis is passing:

https://travis-ci.org/ipfs/js-ipfs-api/builds/371638643

error sha3@1.2.1: The engine "node" is incompatible with this module. Expected version ">= 8.11.1".
error An unexpected error occurred: "Found incompatible module".

[prettymuchbryce]

@diasdavid Mind pointing us in the right direction with the testing issues? Anything we can do to help?

[victorb]

I've rerun the tests on jenkins as it was still using a old nodejs version we switched yesterday. Change was made here: ipfs-inactive/jenkins-libs@54eca5d

I rerun with 8.11.1 activated, testrun is here: https://ci.ipfs.team/blue/organizations/jenkins/IPFS%2Fjs-ipfs-api/detail/PR-741/6/pipeline

[ghost]

I'm not familiar with the http server lib we're using here -- does it transparently convert header keys to camel case?

[OR13]

@lgierth I've often wondered about casing of http headers, especially across languages:

https://stackoverflow.com/questions/5258977/are-http-headers-case-sensitive

Here are come examples of headers being set prior to this change.

https://github.com/ipfs/js-ipfs-api/blob/master/src/utils/send-request.js#L113

Its worth noting that user agent header will be overwritten by this code even if its provided in the config.

The headers object is passed directly to the request library:

https://github.com/ipfs/js-ipfs-api/blob/master/src/utils/send-request.js#L162

https://github.com/ipfs/js-ipfs-api/blob/master/src/utils/request.js

Here is an interesting blog post on header casing and some challenges it can cause:

https://medium.com/walmartlabs/hacking-node-core-http-module-f2a10ea3028e

[OR13]

long story short, i don't think header casing should be altered by code in this library.

[mikeal]

This is actually a pretty hard and annoying problem, particularly in Node.js.

The basic rule of thumb is that you need to preserve casing on user inputs. This is very problematic if you need to check and manipulate headers elsewhere in code.

I wrote a library to deal with this in request, so it's pretty widely used. https://github.com/request/caseless, basically just wraps the headers object and provides an API for you to manipulate it in a caseless way while still maintaining the case of the user inputs.

[OR13]

The test that is failing, passes for me locally, and passes on travis...

  it('.ping with default n', (done) => {
    ipfs.ping(otherId, (err, res) => {
      expect(err).to.not.exist()
      expect(res).to.be.an('array')
      expect(res).to.have.lengthOf(3)
      res.forEach(packet => {
        expect(packet).to.have.keys('Success', 'Time', 'Text')
        expect(packet.Time).to.be.a('number')
      })
      const resultMsg = res.find(packet => packet.Text.includes('Average latency'))
      expect(resultMsg).to.exist()
      done()
    })
  })

Example expected output:

[ { Success: true,
    Time: 0,
    Text: 'PING QmQF7qFBAzMdXr8NyMR7uQV7WYuafmpC1xjCsdbmGZ6PDP.' },
  { Success: true, Time: 225158, Text: '' },
  { Success: true, Time: 0, Text: 'Average latency: 0.23ms' } ]

Not sure what I can do to fix this build issue, any advice is welcome.

I see that its WIP: #762

[alanshaw]

@OR13 If all your tests have passed I think that's something we need to solve, hopefully once we have, a rebase and retest should get this passing. I'll come back to you asap.

[prettymuchbryce]

@alanshaw Any updates here? Would be great to be able to authenticate with our nodes.

[alanshaw]

We need an Object.assign({}, config.headers) to avoid mutating the headers object passed as config, otherwise subsequent requests will receive the same headers as previous.

[alanshaw]

CI is currently having issues, running the tests locally passes. Thank you @OR13 🚀