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 Encoding::UndefinedConversionError for IE browsers #518

Closed
wants to merge 25 commits into from
Closed

Fix Encoding::UndefinedConversionError for IE browsers #518

wants to merge 25 commits into from

Conversation

lucke84
Copy link
Contributor

@lucke84 lucke84 commented Aug 11, 2016

Check for encoding failure of the request's original_url and try to recover by force-encoding the URLs inside the payload as UTF-8, assuming they were encoded in Windows Latin-2.
This situation can only occur in browsers that do not encode the entire URL as UTF-8 already, meaning IE11 and lower. This is quite a common config as many people on Windows 7 still use IE11.

Here's the relevant part of the backtrace:

Encoding::UndefinedConversionError: "\xC3" from ASCII-8BIT to UTF-8
  from active_support/core_ext/object/json.rb:34:in `encode'
  from active_support/core_ext/object/json.rb:34:in `to_json'
  from active_support/core_ext/object/json.rb:34:in `to_json_with_active_support_encoder'
  from active_support/json/encoding.rb:57:in `to_json'
  from json/common.rb:224:in `generate'
  from json/common.rb:224:in `generate'
  from active_support/json/encoding.rb:101:in `stringify'
  from active_support/json/encoding.rb:35:in `encode'
  from active_support/json/encoding.rb:22:in `encode'
  from active_support/core_ext/object/json.rb:37:in `to_json_with_active_support_encoder'
  from react_on_rails_helper.rb:285:in `server_rendered_react_component_html'
  from react_on_rails_helper.rb:112:in `react_component'
  from app/views/hotels/list.html.erb:1:in `_app_views_hotels_list_html_erb___3550728304780922533_32214600'
  [ .. ]  

This is a patch we have already successfully applied to our codebase in other components. Truth to be told, though, I still have to try the fix within the ReactOnRails context. I will do it tomorrow, but I'd appreciate an initial round of feedback. Thanks!

This change is Reviewable

…ecover by force-encoding the URLs inside the payload as utf-8, assuming they were encoded in Windows Latin-2.

This situation can only occur in browsers that do not encode the entire URL as UTF-8 already, meaning IE11 and lower. This is quite a common config as many people on Windows 7 still use IE11.
@justin808
Copy link
Member

@lucke84 Thanks. Looks great. Please see https://github.com/shakacode/style-guide-javascript/blob/master/CONTRIBUTING.md.

We need:

  • Changelog entry.
  • README note? Maybe not
  • test demonstrating the issue and confirming the fix.

Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


app/helpers/react_on_rails_helper.rb, line 360 [r1] (raw file):

        # This is quite a common config as many people on Windows 7 still use IE11.
        begin
          original_url_normalized = request.original_url if request.original_url.present?

I'm guessing that we don't have to declare original_url_normalized outside of the block in ruby... @samnang confirm?


Comments from Reviewable

@justin808
Copy link
Member

@lucke84 If you have any production projects, please consider adding them to https://github.com/shakacode/react_on_rails/blob/master/PROJECTS.md


Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


Comments from Reviewable

@justin808 justin808 added this to the 6.1 milestone Aug 12, 2016
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling b7a6d66 on FindHotel:fix_undefined_conversion_error into * on shakacode:master*.

@lucke84
Copy link
Contributor Author

lucke84 commented Aug 12, 2016

@justin808 Will update the CHANGELOG and, if needed, the README. I'll run our app with our branch to see if it fixes the error properly today, then will codify a test for regression's sake.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 82.219% when pulling 17aa2fb on FindHotel:fix_undefined_conversion_error into a6373f0 on shakacode:master.

@lucke84
Copy link
Contributor Author

lucke84 commented Aug 15, 2016

Quick update: I've greatly simplified the logic for handling the problematic situations. I find it a bit hard to create a test for this issue, the problem seems to be very much related to the Windows platform (mostly IE browsers, but I've seen it happening on Firefox as well). If you try and hit any URL in a ReactOnRails-powered app that contains a weird character in the querystring (e.g. ?xyz=hellø), you will see it crashing.

Maybe somebody here can give it a go and:

  • confirm the issue is there
  • confirm that the solution works
  • come up with a suggestion to test it?

@justin808
Copy link
Member

@lucke84 Please check on why travis is failing.

@justin808
Copy link
Member

@lucke84 please run rake lint

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.06%) to 82.301% when pulling ac92944 on FindHotel:fix_undefined_conversion_error into 52f0614 on shakacode:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.06%) to 82.301% when pulling ac92944 on FindHotel:fix_undefined_conversion_error into 52f0614 on shakacode:master.

@justin808
Copy link
Member

:lgtm:


Reviewed 1 of 1 files at r3, 1 of 1 files at r5.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


Comments from Reviewable

@justin808
Copy link
Member

@lucke84 Please rebase on top of master and I'll merge this.

…ecover by force-encoding the URLs inside the payload as utf-8, assuming they were encoded in Windows Latin-2.

This situation can only occur in browsers that do not encode the entire URL as UTF-8 already, meaning IE11 and lower. This is quite a common config as many people on Windows 7 still use IE11.
…/react_on_rails into fix_undefined_conversion_error
…ecover by force-encoding the URLs inside the payload as utf-8, assuming they were encoded in Windows Latin-2.

This situation can only occur in browsers that do not encode the entire URL as UTF-8 already, meaning IE11 and lower. This is quite a common config as many people on Windows 7 still use IE11.
…/react_on_rails into fix_undefined_conversion_error
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.06%) to 82.301% when pulling 3997a2f on FindHotel:fix_undefined_conversion_error into 52f0614 on shakacode:master.

@justin808
Copy link
Member

@lucke84 Please squash your change down to one tidy commit with a detailed message: http://chris.beams.io/posts/git-commit/

@justin808
Copy link
Member

@lucke84 I'd like to get 6.1 out this weekend. Let me know if you'll have a chance to fix your rebase.

@lucke84
Copy link
Contributor Author

lucke84 commented Aug 21, 2016

@justin808 I've tried the rebase and squash, but couldn't get the output I want (I'm not yet a huge fan of rebase, so I'm a bit out of practice). Will close this PR and open a new one off the latest master, ready to be merged. Is that ok?

@lucke84
Copy link
Contributor Author

lucke84 commented Aug 21, 2016

Created #525 where I've squashed the changes, but something went south there too. Let me know if that's fine or if I should create it again ¯_(ツ)_/¯

@justin808
Copy link
Member

@lucke84 ASAP!

That looks good. But squash to one commit!

@lucke84
Copy link
Contributor Author

lucke84 commented Aug 21, 2016

Closing in favour of #527.

@lucke84 lucke84 closed this Aug 21, 2016
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