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

Content-Type does not respect optional charset encoding for iOS RCTJavascriptLoader #15791

Closed
justinr1234 opened this Issue Sep 4, 2017 · 7 comments

Comments

Projects
None yet
5 participants
@justinr1234
Contributor

justinr1234 commented Sep 4, 2017

Is this a bug report?

Yes

Have you read the Contributing Guidelines?

Yes

Environment

  1. react-native -v: 0.42.0
  2. node -v: v6.11.2
  3. npm -v: 4.6.1
  4. yarn --version: 0.27.5
  • Target Platform: iOS
  • Development Operating System: macOS
  • Build tools: XCode

Steps to Reproduce

  1. Try to run a react native app and download a bundle from a server which returns a Content-Type header that includes both the MIME Type and an optional character encoding as specified here: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Type
  2. The React Native code for iOS will fail on this line of code as it strictly checks the Content-Type header as if the optional character encoding doesn't exist: https://github.com/facebook/react-native/blame/master/React/Base/RCTJavaScriptLoader.mm#L277

Expected Behavior

The code should follow the specification of Content-Type and respect only MIMEType checking instead of strictly checking the Content-Type header for an exact match on MIMEType.

Per Apple documentation, NSURLResponse contains the MIMEType field that should be checked for application/javascript instead of checking the raw Content-Type header: https://developer.apple.com/documentation/foundation/nsurlresponse

Actual Behavior

Though this was encountered while using Expo, the error is nonetheless coming from the RCTJavaScriptLoader

screen shot 2017-09-04 at 7 52 51 am

Expected JavaScript, but got content type 'application/javascript; chartset=UTF-8'. Check that your internet connection is working.

Reproducible Demo

https://github.com/sysgears/apollo-universal-starter-kit

Follow starter instructions and checkout this commit: sysgears/apollo-universal-starter-kit@bbcd52a

The problem is worked around after this commit in the project.

@joearasin

This comment has been minimized.

Show comment
Hide comment
@joearasin

joearasin Sep 4, 2017

I just tripped over this when attempting to get Haul to make a build for react-native 0.48.2: callstack/haul#228

joearasin commented Sep 4, 2017

I just tripped over this when attempting to get Haul to make a build for react-native 0.48.2: callstack/haul#228

@anp

This comment has been minimized.

Show comment
Hide comment
@anp

anp Sep 5, 2017

Collaborator

cc'ing @janicduplessis as he was cc'd on an Expo bug report.

Collaborator

anp commented Sep 5, 2017

cc'ing @janicduplessis as he was cc'd on an Expo bug report.

@grabbou

This comment has been minimized.

Show comment
Hide comment
@grabbou

grabbou Sep 11, 2017

Collaborator

Up @janicduplessis, there are some commits in the history of this issue. Is any of them ready for merging? If yes, I can go ahead and cherry-pick into respective branches.

Collaborator

grabbou commented Sep 11, 2017

Up @janicduplessis, there are some commits in the history of this issue. Is any of them ready for merging? If yes, I can go ahead and cherry-pick into respective branches.

@janicduplessis

This comment has been minimized.

Show comment
Hide comment
@janicduplessis

janicduplessis Sep 11, 2017

Collaborator

The PR has not been merged yet #15792, let's cherry pick it once it is.

Collaborator

janicduplessis commented Sep 11, 2017

The PR has not been merged yet #15792, let's cherry pick it once it is.

facebook-github-bot added a commit that referenced this issue Sep 20, 2017

Fix Content-Type header checking of React/RCTJavascriptLoader.mm #15791
Summary:
To fix this issue: #15791
Closes #15792

Differential Revision: D5813101

Pulled By: shergin

fbshipit-source-id: fd3eb6f1d9ccdeb5373d1ba2b2df173ff7a8e986
@stale

This comment has been minimized.

Show comment
Hide comment
@stale

stale bot Nov 10, 2017

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Maybe the issue has been fixed in a recent release, or perhaps it is not affecting a lot of people. If you think this issue should definitely remain open, please let us know why. Thank you for your contributions.

stale bot commented Nov 10, 2017

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Maybe the issue has been fixed in a recent release, or perhaps it is not affecting a lot of people. If you think this issue should definitely remain open, please let us know why. Thank you for your contributions.

@stale stale bot added the Stale label Nov 10, 2017

@justinr1234

This comment has been minimized.

Show comment
Hide comment
@justinr1234

justinr1234 Nov 10, 2017

Contributor

@janicduplessis did this end up getting merged somewhere?

Contributor

justinr1234 commented Nov 10, 2017

@janicduplessis did this end up getting merged somewhere?

@stale stale bot removed the Stale label Nov 10, 2017

@stale

This comment has been minimized.

Show comment
Hide comment
@stale

stale bot Jan 9, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Maybe the issue has been fixed in a recent release, or perhaps it is not affecting a lot of people. If you think this issue should definitely remain open, please let us know why. Thank you for your contributions.

stale bot commented Jan 9, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Maybe the issue has been fixed in a recent release, or perhaps it is not affecting a lot of people. If you think this issue should definitely remain open, please let us know why. Thank you for your contributions.

@stale stale bot added the Stale label Jan 9, 2018

@stale stale bot closed this Jan 16, 2018

@facebook facebook locked and limited conversation to collaborators May 15, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.