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

Failed to parse header with github fetch polyfill library, response.headers.map won't work correctly #11128

Closed
younthu opened this issue Nov 25, 2016 · 9 comments
Labels
Resolution: Locked This issue was locked by the bot.

Comments

@younthu
Copy link
Contributor

younthu commented Nov 25, 2016

Issue Description
I have a react native project, pure react native project for android, it has been under development for few months, it works well until i re-installed packages recently.

After i re-install node_modules with npm install, the network module won't work correctly anymore. It will failed to parse the response's headers correctly, below code will get undefined
let {'access-token' : accessToken, client, uid, 'token-type':tokenType} = response.headers.map;

Where the bug is
I dig into the code, and found the root cause is in react-native/node_modules/whatwg-fetch/fetch.js, line 366:

function parseHeaders(rawHeaders) {
    var headers = new Headers()
    rawHeaders.split('\r\n').forEach(function(line) {// line 366, error happens here, 
                          // it won't split the headers correctly, returns the whole headers as one line. 
                         // it works after i change it to rawHeaders.split('\n').forEach(....){
      var parts = line.split(':')
      var key = parts.shift().trim()
      if (key) {
        var value = parts.join(':').trim()
        headers.append(key, value)
      }
    })
    return headers
  }

So the split('\r\n') does not work correctly, lead to response.headers.map messed up. My app's authentication depends on tokens from response.headers, it is broken by this error.

What lead to this bug
I compared the react-native/node_modules folders before my reinstall, i found there are something changed after i re-install package.

Before i re-install package, there is no react-native/node_modules/whatwg-fetch, after i reinstall it, the react-native/node_modules/whatwg-fetch comes in, then, my app's network module broken.

Suggestion
Remove whatwg-fetch, or fix the split bug in fetch library.

My enviroment

react-native-cli: 1.0.0
react-native: 0.34.1

"react": "15.3.1",
 "react-native": "^0.34.1",
node v6.2.2
@ide
Copy link
Contributor

ide commented Nov 25, 2016

It sounds like your server is buggy, you're supposed to use \r\n between headers. See http://stackoverflow.com/a/5757349/454967. That link also says that client software can optionally choose to accept \n as well but that change would need to be made to GitHub's library, so you should convince them to make that change.

@younthu
Copy link
Contributor Author

younthu commented Nov 25, 2016

@ide

I captured the response with wireshark, the headers' line break is '\r\n' for sure. I believe it is the github fetch library's bug. Our server is nginx + passenger, RoR solution, i don't think the server has this kind of bug.

For change's to github fetch library, i will try it. But before that, can we remove dependency for github fetch, drop back to previous fetch implementation?

BTW, What's the previous fetch implementation please?

If you need a sample project to repro this issue, please let me know, i can try make one and upload to github.

Thanks.

@mkonicek
Copy link
Contributor

mkonicek commented Nov 27, 2016

We've depended on whatwg-fetch for a very long time (last change to the corresponding line of package.json in August): https://github.com/facebook/react-native/blame/5d30045211ff1d97cd7005bc268442f3df998b17/package.json

Any idea why it only changed for you now? What was your previous RN version?

I'm not sure what we used before whatwfg-fetch unfortunately.

@younthu
Copy link
Contributor Author

younthu commented Nov 27, 2016

@mkonicek , let me try to repro this issue with a new project. I will notify you once i cloned this issue to a new RN project.

react-native-cli: 1.0.0
react-native: 0.34.1

"react": "15.3.1",
 "react-native": "^0.34.1",
node v6.2.2

@younthu
Copy link
Contributor Author

younthu commented Nov 27, 2016

@mkonicek & @ide ,

I uploaded a test project in github to repro this issue, would you like to have a look?

If need any info, please let me know.

@finalquest
Copy link

Same issue here.
Fetch is not splitting the headers.
Last week the version of whatwg-fetch installed with react was 1.0.0.
Today is 1.1.1.
Forcing the module included in the react-native package.json to "whatwg-fetch": "1.0.0" (prev was "whatwg-fetch": "^1.0.0",), solved the issue

@fson
Copy link
Contributor

fson commented Jan 3, 2017

whatwg-fetch@1.1.0 and newer split the headers by \r\n as per spec. This behavior was introduced in JakeChampion/fetch@57565a1.

The XMLHttpRequest.getAllResponseHeaders() method in React Native used to return the headers joined by \n, but this was fixed in v0.36.0 (24c72f5). A workaround for older React Native versions is to add a dependency on whatwg-fetch@1.0.0 which forces that exact version to be installed.

@younthu
Copy link
Contributor Author

younthu commented Jan 4, 2017

Thanks to @fson, i've upgraded the react-native to 0.39 and it is resolved. for those who are using v0.36.0, strongly recommended to upgrade the reac-native to newer than 0.36, or, fix as @fson suggested.

@mkonicek , will this issue be patched in v0.36? Should we keep this issue closed or open for later users?

@hramos
Copy link
Contributor

hramos commented May 25, 2017

Closing this issue because it has been inactive for a while. If you think it should still be opened let us know why.

@hramos hramos closed this as completed May 25, 2017
@hramos hramos added the Icebox label May 26, 2017
@facebook facebook locked as resolved and limited conversation to collaborators Jul 19, 2018
@react-native-bot react-native-bot added the Resolution: Locked This issue was locked by the bot. label Jul 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Resolution: Locked This issue was locked by the bot.
Projects
None yet
Development

No branches or pull requests

7 participants