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

do not set ContentLength to NaN #707

Closed
wants to merge 2 commits into from
Closed

Conversation

yaacovCR
Copy link
Contributor

@yaacovCR yaacovCR commented Dec 27, 2019

node-fetch should take advantage of @naugtur NaN workaround for the form-data package, see form-data/form-data#397 (comment).

form-data package implementation of FormData allows for only two situations: streams of user pre-specified "knownLength" or streams with defined length retrievers (file streams with ends defined, files with sizes on disk, http requests with a content-length), but no concept of a regular old stream with an unknownable length.

naugtur suggests prespecifying the knownLength of such a stream as NaN, which will flow into the calculations and the eventual result of FormData.prototype.getLengthSync.

naugtur notes that this will cause the request package to not set the ContentLength header, and could even be considered a FormData feature.

node-fetch should support this "feature" as well.

@codecov-io
Copy link

codecov-io commented Dec 27, 2019

Codecov Report

Merging #707 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #707   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           7      7           
  Lines         579    579           
  Branches      185    185           
=====================================
  Hits          579    579
Impacted Files Coverage Δ
src/request.js 100% <100%> (ø) ⬆️

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 8c197f8...9b28c7f. Read the comment docs.

@yaacovCR
Copy link
Contributor Author

Related: form-data/form-data#399

@naugtur
Copy link

naugtur commented Dec 27, 2019

I'm famous! 😄

Yes, the NaN hack seems the only way to push the "unknowable length" through the function calls down to form-data so that both form-data and request work correctly.

I must add one thing: a === a is not the easiest to understand NaN check.
I'd suggest this https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/isNaN

@yaacovCR
Copy link
Contributor Author

So famous!!!

Agree, isNaN is better. Thoughts from @bitinn ?

@Richienb
Copy link
Member

Please open PRs against the 3.x branch.

@yaacovCR
Copy link
Contributor Author

On further reflection, I think the underlying "right" thing is for FormData to not rely on use of NaN, but just to support streams of unknowable length out of the box.

See further discussion (hopefully!) at form-data/form-data#394.

Although I guess still appropriate for node-fetch to check NaN, too. I will open a PR against the 3.0.0 branch as well.

@Richienb
Copy link
Member

Moved to #709.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants