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

Look at dropping superagent, in favour of fetch? #203

Open
keithamus opened this issue Feb 6, 2018 · 12 comments
Open

Look at dropping superagent, in favour of fetch? #203

keithamus opened this issue Feb 6, 2018 · 12 comments

Comments

@keithamus
Copy link
Member

keithamus commented Feb 6, 2018

Firstly, no shade toward superagent. It's an excellent library and the maintainers should be proud of the product they've made. But I think it could be useful to explore dropping it as the backing library for chai-http.

I'd say around 40-50% of the issues we get on this tracker are related to features or bugs within superagent. Many around how to use superagent and its API, which we do not document so well. These days we have a well-known, elegant, promise based lib in the form of fetch - which we could potentially use as a replacement for superagent.

What does superagent offer us over fetch?

Most of what SuperAgent offers us - over fetch - is terseness in sending complex requests, and reviewing their contents. Take a look:

// SuperAgent:
const response = await request
  .post('/api/pet')
  .type('form')
  .send({ name: 'Manny', species: 'cat' }) // sends a JSON post body
  .set('X-API-Key', 'foobar')
  .set('accept', 'application/json')
expect(response).to.have.status(200)
expect(response).to.have.header('Content-Type', 'application/json')
expect(response.body.id).to.equal('1234')
// Fetch:
const body = new FormData()
body.append('name', 'Manny')
body.append('species', 'cat')
const response = await fetch('/api/pet', {
  method: 'post',
  body,
  headers: { ...body.getHeaders(), 'X-API-Key': 'foobar', 'Accept': 'application/json' }
})
expect(response).to.have.status(200)
expect(response).to.have.header('Content-Type', 'application/json')
const json = await response.json()
expect(json).to.equal('1234')

One problem here is that fetch gives you the basics, but meanwhile to send data around you'll potentially need to learn the apis for URL, URLSearchParams, FormData. This also means for Node we'll need to ensure those libs exist with polyfills for older engines (URL and URLSearchParams shipped in Node 7).

One big benefit is - despite the extra verbosity of the code, we can rely on the implicit knowledge of the developer knowing the fetch api - which I would bet most users know over superagent. When a user doesn't know these APIs we can point them to the wealth of information available on quality sites like mdn.

I'd like to get opinions from users who use chai-http. I use it myself and I'd be pretty happy switching to fetch - as it covers most of my usecases, but I'd like to hear from the community if this is a worthwhile thing to do.

@JamesMessinger
Copy link
Member

JamesMessinger commented Feb 6, 2018

I really like the syntactic sugar that Superagent provides. It feels consistent with Chai's BDD syntax, which makes test code look coherent and consistent.

Doesn't Superagent use Fetch under the hood (at least when running in a browser)? If so, then there's already some code somewhere inside Superagent to convert a Fetch response to the Superagent response object, which is what Chai-HTTP relies on for its assertions. Maybe we could just re-use that code.

Another option is to not include any request library with Chai-HTTP (Fetch, Superagent, or otherwise). That way, Chai-HTTP's would just be an assertion library that operates on a standardized representation of a "response" object. Then we'd just need some way of converting response objects from various request libraries into the standardized format. We could provide built-in conversion logic for a few popular libraries (Fetch, Superagent, Axios, etc.) and allow users to provide their own conversion "plug-ins" for other libraries.

Or maybe I'm over-complicating things. 🤷‍♂️

@keithamus
Copy link
Member Author

Another option is to not include any request library

This is tempting. Of course it would mean every one of our existing assertions (which are made to support only superagent would need to support a myriad of clients, I presume each time trying to detect which one is which. It also forces users to then decide which library they should pick to run these assertions, and so this becomes a less "complete" plugin. The benefit of moving to fetch is that we keep feature completeness but do not sacrifice the utility of this plugin in a major way.

Maybe we could just re-use that code

I don't know how much fetch uses SuperAgent - but this is also another excellent suggestion. We could switch to fetch, but still supply a set of convenience methods for common operations. Or have a lightweight layer over fetch that aligns better with chai-http opinions. This way we get the best of both worlds.

@JamesMessinger
Copy link
Member

The benefit of moving to fetch is that we keep feature completeness but do not sacrifice the utility of this plugin in a major way

Great point. And that's a very important design goal

We could switch to fetch, but still supply a set of convenience methods for common operations. Or have a lightweight layer over fetch that aligns better with chai-http opinions.

That sounds like the best route. It'll make Chai-HTTP feel more complete and cohesive, and it'll also provide a smoother upgrade path for folks who want to migrate from the old syntax without completely replacing it with Fetch

@wheresrhys
Copy link

Could steal some ideas from https://github.com/mikeal/r2

@leggsimon
Copy link
Contributor

One problem here is that fetch gives you the basics, but meanwhile to send data around you'll potentially need to learn the apis for URL, URLSearchParams, FormData. This also means for Node we'll need to ensure those libs exist with polyfills for older engines (URL and URLSearchParams shipped in Node 7).

How about a vaguely happy medium where you use fetch but provide some helpers for those APIs? Downside is it is maybe slightly more to maintain but if they were "sugar" around simple forms or something eg:

// no helpers
const body = new FormData()
body.append('name', 'Manny')
body.append('species', 'cat')
const response = await fetch('/api/pet', {
  method: 'post',
  body,
  headers: { ...body.getHeaders(), 'X-API-Key': 'foobar', 'Accept': 'application/json' }
})

// with a simple form helper
const body = createFormBody({ name: 'Manny', species: 'cat' })

const response = await fetch('/api/pet', {
  method: 'post',
  body,
  headers: { ...body.getHeaders(), 'X-API-Key': 'foobar', 'Accept': 'application/json' }
})

@keithamus
Copy link
Member Author

Maybe - following that idea @leggsimon - we could offer fetch, but also tack on a bunch of helper methods that close over the fetch function signature, similar existing request libraries. For example:

chai.fetch(url, ...) // this is just normal `fetch`

chai.fetch.put(url, {}) // this calls `fetch(url, { method: 'put' })`

chai.fetch.form(url, {
  body: { name: 'Manny', species: 'cat' },
  headers: { ...body.getHeaders(), 'X-API-Key': 'foobar', 'Accept': 'application/json' }
}) 
// ^ this is the equivlanent of :

const body = new FormData()
body.append('name', 'Manny')
body.append('species', 'cat')
chai.fetch(url, method: 'post',
  body,
  headers: { ...body.getHeaders(), 'X-API-Key': 'foobar', 'Accept': 'application/json' }
})

@acme
Copy link

acme commented Mar 5, 2018

I'm mostly using chai with fetch() thesedays, so I'm generally positive about this proposal.

I note that http://chaijs.com/plugins/chai-fetch/ and http://chaijs.com/plugins/node-fetch-response-matchers/ already exist.

Perhaps rather than moving chai-http to a different backend we could instead deprecate the module and build a newer, shiner one?

@kriscampos
Copy link

I think its worth pointing out that superagent is known to be problematic when developing server code in TypeScript. The solutions discussed in that thread have their own downsides and I actually first encountered these problems through chai-http.

Maybe we could use a project like node fetch for server-side use cases?

@austince
Copy link
Contributor

austince commented Sep 5, 2018

Has any dev work been started on that yet? If not, I'll see if I have time to get something started this weekend!

@keithamus
Copy link
Member Author

keithamus commented Sep 6, 2018

@austince if you want to - head onto our Slack instance and we can chat about the direction I'd like to see this work go in.

@L2jLiga
Copy link

L2jLiga commented Feb 6, 2020

Hi there! Any updates on it?

@austince
Copy link
Contributor

austince commented Feb 6, 2020

Hi @L2jLiga - I got about 75% done with a library that might be a suitable replacement but have never quite found the time to take it over the finish line: https://github.com/austince/fluently-fetch/tree/feat/immutible

If you're interested in contributing to that/ taking another approach, I would be grateful!

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

No branches or pull requests

8 participants