README 2.x update proposal... take 1 #448

jkantr · 2018-05-01T03:06:27Z

Just getting some feelers out there to see if this sort of thing would be received favorably. I reworked some of the structure and content of the docs, tried to make them a little more beginner friendly, take a few less liberties, be a little more consistent, and urge some best practices.

Let me know if you all think it's going in the right direction...

OH, right, also I have it identical here in a gist, if this is easier than trying to see the markdown otherwise lol:

https://gist.github.com/jkantr/1b7bcc0e660548476fbc2f184467c41e

(note, this doesn't directly refer to the points in #419 as tbh i wasn't super clear what those updates were meant for... but I could work them in I'm sure)

bitinn · 2018-05-01T04:47:02Z

Great job Jared! Rewriting manual is a tiresome job but they are critical to users, thank you for taking the time to do this.

Formatting and readability wise I think this is much better than the current one. @TimothyGu @jimmywarting @gr2m if you got time, give it a quick read and let us know if there are technical details you want to amend?

A few suggestions:

You seem to be editing an older version of the readme? Some recent changes appear to be lost here (which might also explain why the PR is conflicting?)

See Matt Andrews' isomorphic-fetch or Leonardo Quixada's cross-fetch for isomorphic usage (exports node-fetch for server-side, whatwg-fetch for client-side).

The Table of Content could be shorter? Personally I would take out 3rd level items to make it shorter.
textConverted API should mention an optional dependency on encoding, which users need to install by themselves, and this is done purely for backward compatibility with 1.x release.
we always said content-length will be "automatically calculated, if possible" in the default header section, but we never explain what's the condition for it to be calculated, and that chunked transfer-encoding will be used when they are not calculated or supplied.

As for my original PR #419 , I was trying to flag some issues I would like to work on (but never did).

I have summarized issues related to readme above.
Add bodyUsed limitation to Known Differences doc #402 is for known limits doc, not directly related to readme doc.
I am closing The readme update discussion PR #419 in favorite of this PR.

jkantr · 2018-05-01T04:50:02Z

You seem to be editing an older version of the readme?

Oh dear, don't I feel sheepish.. usually I'm pretty good with making sure I'm at head... I'll get that merged in properly asap o.O

Good points all around, I'll dig into each when I find a few moments :) and will await input from others as well~

Thanks for the response!

EDIT: okay, should be merging properly now / all update to date info is there.. will work on suggestions next

codecov-io · 2018-05-01T05:21:17Z

Codecov Report

Merging #448 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff          @@
##           master   #448   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           6      6           
  Lines         503    503           
  Branches      158    158           
=====================================
  Hits          503    503

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 09ef40e...522e27d. Read the comment docs.

jimmywarting · 2018-05-01T09:32:51Z

If you prefer to cache binary data in full, use buffer(). (NOTE: buffer() is a node-fetch only API)

Somewhere i think we also should mention arrayBuffer() also if you want to be cross-fetch compatible.

Use native stream for body, on both request and response.

I feel conflicted of what is a native stream now that both node and browser has 2 different spec.
Seeing it as the fetch api is more of a web thing then it is a NodeJS thing i could maybe believe it would giving me a whatwg stream since that is what is used in the browser.
Think they should be called Node stream or whatwg stream to distinguish those to

TimothyGu

Awesome work! Some comments follow.

TimothyGu · 2018-05-02T23:41:34Z

README.md

+
+#### Simple Post
+```javascript
+fetch('http://httpbin.org/post', { method: 'post', body: 'a=1' })


I would encourage uppercasing the method names.

Minor thing that bugs me is the use of http and protocol relative urls. Should use https whenever possible to stop spying mitm attacks. Saying this cuz i know that httpbin has https

TimothyGu · 2018-05-02T23:42:04Z

README.md

+
+## Loading and configuring the module
+We suggest you load the module via `require`, pending the stabalizing of es modules in node:
+```javascript


Can we be consistent about the language tag? I'd prefer just js.

TimothyGu · 2018-05-02T23:42:19Z

README.md

+## Features
+
+- Stay consistent with `window.fetch` API.
+- Make conscious trade-off when following [whatwg fetch spec][whatwg-fetch] and [stream spec](https://streams.spec.whatwg.org/) implementation details, document known difference.


nit: WHATWG

TimothyGu · 2018-05-02T23:43:39Z

README.md

+#### Plain text or HTML
+```javascript
+fetch('https://github.com/')
+    .then(res => res.text())


Style-wise, the code base uses tabs for indenting. I understand why you changed it to spaces, but I feel two spaces is more common in the JavaScript world anyway.

TimothyGu · 2018-05-02T23:45:43Z

README.md

+`Accept`          | `*/*`
+`Connection`      | `close` _(when no `options.agent` is present)_
+`Content-Length`  | _(automatically calculated, if possible)_
+`User-Agent`      | `node-fetch/1.0 (+https://github.com/bitinn/node-fetch)`


Maybe also add Transfer-Encoding: chunked? That header is added by Node.js automatically if the input is a stream, I believe.

TimothyGu · 2018-05-02T23:46:23Z

README.md

+Because Node.js does not implement service workers (for which this class was designed), one rarely has to construct a `Response` directly.
+
+#### response.ok
+


This one is also spec-compliant.

TimothyGu · 2018-05-02T23:47:31Z

README.md

+==========
+
+[![npm stable version][npm-image]][npm-url]	
+[![npm next version][npm-next-image]][npm-url]


I don't think we need a next version any more, not until we prepare for v3.x.

TimothyGu · 2018-05-02T23:51:04Z

Might be good to also mention the fact that we don't do any proxy business, à la #449.

bitinn · 2018-05-03T08:45:47Z

It might be a good idea to reformat the options section into a table layout (like the headers section), instead of current code block.

Mainly because agent is an easily overlooked feature that we support since v1, and you need it for:

proxy
self-signed certificate
keep alive
possibly other low level network stuffs

Using a table might give it a bit more awareness and give us more space to write (code block isn't great for line wrapping)

gr2m

looks good to me, just a few comments :)

gr2m · 2018-05-03T21:44:58Z

README.md

+
+Instead of implementing `XMLHttpRequest` in Node.js to run browser-specific [Fetch polyfill](https://github.com/github/fetch), why not go from native `http` to `fetch` API directly? Hence `node-fetch`, minimal code for a `window.fetch` compatible API on Node.js runtime.
+
+See Matt Andrews' [isomorphic-fetch](https://github.com/matthew-andrews/isomorphic-fetch) or Leonardo Quixada's [cross-fetch](https://github.com/lquixada/cross-fetch) for isomorphic usage (exports `node-fetch` for server-side, `whatwg-fetch` for client-side).


It is unclear to what the linked projects are. At first I thought they would be similar projects to node-fetch, but in fact they depend on node-fetch. Also isn’t node-fetch itself already isomorphic?

Maybe instead of the paragraph above, say something like this:

In case you need to support older browsers without fetch support, you can isomorphic-fetch. cross-fetch also adds support React Native.

gr2m · 2018-05-03T21:48:43Z

README.md

+
+## Motivation
+
+Instead of implementing `XMLHttpRequest` in Node.js to run browser-specific [Fetch polyfill](https://github.com/github/fetch), why not go from native `http` to `fetch` API directly? Hence `node-fetch`, minimal code for a `window.fetch` compatible API on Node.js runtime.


I think the main motivation is to reduce the size of the code that is shipped to browsers as it matters much more than it does in Node. I’m not very good with English myself, but maybe something like this:

Code size matters much more in browsers than it does in Node, so instead of implementing polyfills for node APIs in the browser, node-fetch implements a polyfill for the browser-native fetch API in the browser. No additional code needs to be send to browsers when using node-fetch in your code.

gr2m · 2018-05-03T21:55:10Z

README.md

+
+## Features
+
+- Stay consistent with `window.fetch` API.


I’d maybe remove the verb, just say "consistent with window.fetch API."

Or maybe rename "Features" to "Goals" or "Scope"?

gr2m · 2018-05-03T22:34:11Z

README.md

+
+## Installation
+
+Current stable release (`2.x`)


maybe remove that? The npm badge on top shows the version, we could also move it here

gr2m · 2018-05-03T22:38:52Z

README.md

+
+[whatwg-fetch]: https://fetch.spec.whatwg.org/
+[response-init]: https://fetch.spec.whatwg.org/#responseinit
+[node-readable]: https://nodejs.org/api/stream.html#stream_readable_streams


I would move License below Acknowledgement

bitinn · 2018-05-25T05:51:19Z

@jkantr I know the suggestions are a bit overwhelming, if it's too much to fix, let's work on the factual updates first, and leave other suggestion to a later PR :)

added "comments" (TODO link references) for changes suggested but not yet implemented for future discussion/prs clarified "native stream" to be "native Node streams" adjusted all uses of http to https to encourage secure protocol usage adjusted whatwg to proper case, WHATWG made code block tags consistent as `js` instead of `javascript` uppercased all method option values (post vs POST) added spec-compliant node to the `response.ok` api section

jkantr · 2018-08-15T15:42:05Z

Sorry I've been invisible for awhile, trying to surface.

Unfortunately in switching between devices and making updates in a less-than-ideal environment, I think I once again have merged the changes in a very non-optimal way. At this point should I just blow away this branch and submit a new PR with just the changes in their current state?

Edit: Okay wow, I don't even know how I managed that. This is some impressively broken rebasing lol.

Edit2: Alright i rewrote the rebase to a merge and i think i've kept the history intact in a reasonable way... let me know

Edit3: Nope, not salvageable... will open a new PR shortly

added "comments" (TODO link references) for changes suggested but not yet implemented for future discussion/prs clarified "native stream" to be "native Node streams" adjusted all uses of http to https to encourage secure protocol usage adjusted whatwg to proper case, WHATWG made code block tags consistent as `js` instead of `javascript` uppercased all method option values (post vs POST) added spec-compliant node to the `response.ok` api section

into readme_2x_updates

added "comments" (TODO link references) for changes suggested but not yet implemented for future discussion/prs clarified "native stream" to be "native Node streams" adjusted all uses of http to https to encourage secure protocol usage adjusted whatwg to proper case, WHATWG made code block tags consistent as `js` instead of `javascript` uppercased all method option values (post vs POST) added spec-compliant node to the `response.ok` api section

jkantr · 2018-08-15T16:20:51Z

Closed in favor of #504

* v2.x readme overhaul with additions discussed in #448 added "comments" (TODO link references) for changes suggested but not yet implemented for future discussion/prs clarified "native stream" to be "native Node streams" adjusted all uses of http to https to encourage secure protocol usage adjusted whatwg to proper case, WHATWG made code block tags consistent as `js` instead of `javascript` uppercased all method option values (post vs POST) added spec-compliant node to the `response.ok` api section * fix left over cruft, inconsistent hierarchy

fix broken indentation

c0f99f5

jkantr added the doc label May 1, 2018

bitinn mentioned this pull request May 1, 2018

The readme update discussion PR #419

Closed

jkantr added 4 commits May 1, 2018 01:10

fix broken indentation

1a26a75

smaller font for deep headers, extra level of hierarchy

54c3eb4

found and merged all the changes i accidentally pruned.. whoops

5b105f3

oops.. missed one

ef532c0

TimothyGu reviewed May 2, 2018

View reviewed changes

gr2m reviewed May 3, 2018

View reviewed changes

bitinn mentioned this pull request May 16, 2018

Add badge to display install size #455

Merged

bitinn mentioned this pull request May 25, 2018

Added support for ArrayBufferView #457

Merged

bitinn mentioned this pull request May 30, 2018

Node.js getaddrinfo ENOTFOUND only when requesting with node-fetch #449

Closed

jkantr added 4 commits August 15, 2018 10:51

fix broken indentation

451e580

fix broken indentation

dc1dbfc

smaller font for deep headers, extra level of hierarchy

f330d90

jkantr added 2 commits August 15, 2018 11:48

Merge branch 'readme_2x_updates' of https://github.com/jkantr/node-fetch

522e27d

into readme_2x_updates

jkantr mentioned this pull request Aug 15, 2018

README 2.x update proposal... take 2 #504

Merged

jkantr closed this Aug 15, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

README 2.x update proposal... take 1 #448

README 2.x update proposal... take 1 #448

jkantr commented May 1, 2018

bitinn commented May 1, 2018

jkantr commented May 1, 2018 •

edited

Loading

codecov-io commented May 1, 2018 •

edited

Loading

jimmywarting commented May 1, 2018

TimothyGu left a comment

TimothyGu May 2, 2018

jimmywarting May 3, 2018

TimothyGu May 2, 2018

TimothyGu May 2, 2018

TimothyGu May 2, 2018

TimothyGu May 2, 2018

TimothyGu May 2, 2018

TimothyGu May 2, 2018

TimothyGu commented May 2, 2018

bitinn commented May 3, 2018

gr2m left a comment

gr2m May 3, 2018

gr2m May 3, 2018

gr2m May 3, 2018

gr2m May 3, 2018

gr2m May 3, 2018

gr2m May 3, 2018

gr2m May 3, 2018

bitinn commented May 25, 2018

jkantr commented Aug 15, 2018 •

edited

Loading

jkantr commented Aug 15, 2018

		Because Node.js does not implement service workers (for which this class was designed), one rarely has to construct a `Response` directly.

		#### response.ok


		Instead of implementing `XMLHttpRequest` in Node.js to run browser-specific [Fetch polyfill](https://github.com/github/fetch), why not go from native `http` to `fetch` API directly? Hence `node-fetch`, minimal code for a `window.fetch` compatible API on Node.js runtime.

		See Matt Andrews' [isomorphic-fetch](https://github.com/matthew-andrews/isomorphic-fetch) or Leonardo Quixada's [cross-fetch](https://github.com/lquixada/cross-fetch) for isomorphic usage (exports `node-fetch` for server-side, `whatwg-fetch` for client-side).


		## Motivation

		Instead of implementing `XMLHttpRequest` in Node.js to run browser-specific [Fetch polyfill](https://github.com/github/fetch), why not go from native `http` to `fetch` API directly? Hence `node-fetch`, minimal code for a `window.fetch` compatible API on Node.js runtime.

README 2.x update proposal... take 1 #448

README 2.x update proposal... take 1 #448

Conversation

jkantr commented May 1, 2018

bitinn commented May 1, 2018

jkantr commented May 1, 2018 • edited Loading

codecov-io commented May 1, 2018 • edited Loading

Codecov Report

jimmywarting commented May 1, 2018

TimothyGu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimothyGu commented May 2, 2018

bitinn commented May 3, 2018

gr2m left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bitinn commented May 25, 2018

jkantr commented Aug 15, 2018 • edited Loading

jkantr commented Aug 15, 2018

jkantr commented May 1, 2018 •

edited

Loading

codecov-io commented May 1, 2018 •

edited

Loading

jkantr commented Aug 15, 2018 •

edited

Loading