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

fix: simplify http client #35

Merged
merged 1 commit into from
Apr 9, 2020
Merged

fix: simplify http client #35

merged 1 commit into from
Apr 9, 2020

Conversation

achingbrain
Copy link
Member

@achingbrain achingbrain commented Apr 9, 2020

The conventions in the Fetch API is that you make a request, then do something with the response:

const response = await fetch('...')
const data = await response.json()

It doesn't do things like:

const data = await fetch.json('...') // what method would this use?

This PR brings our API more inline with Fetch so where we used to do:

for await (const datum of http.ndjson('...')) { // what method does this use?

}

We now do the more idiomatic:

const response = await http.post('...')

for await (const datum of response.ndjson()) {

}

It also removes the .iterator and .stream methods as they do not follow the Fetch pattern either though they can be added to the response object if they are useful.

Copy link

@jacobheun jacobheun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, can we denote this as a BREAKING CHANGE on squash

The conventions in the Fetch API is that you make a request, then do something
with the response:

```javascript
const response = await fetch('...')
const data = await response.json()
```

It doesn't do things like:

```javascript
const data = await fetch.json('...') // what method would this use?
```

This PR brings our API more inline with Fetch so where we used to do:

```javascript
for await (const datum of http.ndjson('...')) { // what method does this use?

}
```

We now do the more idiomatic:

```javascript
const response = await http.post('...')

for await (const datum of response.ndjson()) {

}
```

It also removes the `.iterator` and `.stream` methods as they do not
follow the Fetch pattern either though they can be added to the response
object if they are useful.

BREAKING CHANGE:

- The `.ndjson`, `.stream` and `.iterator` methods have been removed
- An `.ndjson` async generator function has been added to the response which
  does the same thing the `.ndjson` instance method used to

Old:

```javascript
for await (const datum of http.ndjson('http://...')) {

}
```

New:

```javascript
const response = await http.post('http://...')

for await (const datum of response.ndjson()) {

}
```
@achingbrain
Copy link
Member Author

I've updated the commit message to include a BREAKING CHANGE section

@hugomrdias hugomrdias merged commit 05c4c5d into master Apr 9, 2020
@hugomrdias hugomrdias deleted the fix/simplify-http-client branch April 9, 2020 18:27
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.

3 participants