Fix "EOFError error" when using webpack dev server over HTTPS #113

kylefox · 2020-02-12T21:25:20Z

This PR fixes an EOFError: end of file reached error that occurs when running webpack-dev-server over HTTPS (related to the recent changes introduced in #111).

kylefox · 2020-02-12T21:28:06Z

lib/inline_svg/webpack_asset_finder.rb

-          raise
-        end
+        dev_server_asset(@asset_path)
+      elsif Webpacker.config.public_path.present?


I wonder if we can get rid of this check. I can't think of a scenario where an application using webpacker would not have Webpacker.config.public_path configured. 🤔

kylefox · 2020-02-12T21:34:35Z

It would probably be a good idea if the folks from #101 and #111 could verify that this change does not break their setup...

cc @connorshea @jclusso @moxie @dennmart @agrobbin @jamesmartin

(Sorry about all the mentions — just want to ensure I didn't break your development environments!)

kylefox · 2020-02-12T21:36:18Z

lib/inline_svg/webpack_asset_finder.rb

+    def fetch_from_dev_server(file_path)
+      http = Net::HTTP.new(Webpacker.dev_server.host, Webpacker.dev_server.port)
+      http.use_ssl = Webpacker.dev_server.https?
+      http.verify_mode = OpenSSL::SSL::VERIFY_NONE


VERIFY_NONE should be sufficient here, since the webpack dev server should be run in development environments only.

jamesmartin

I don't have a strong opinion about this one because I'm not heavily using Webpacker in any projects right now. I'm a little concerned how large and complex this file is getting without any tests.

I've been considering writing up some integration tests that spin up actual Rails projects (different versions) and do some basic smoke tests, otherwise I have to do this all manually. 🤔

kylefox · 2020-02-12T22:49:30Z

I'm a little concerned how large and complex this file is getting without any tests.

I've been considering writing up some integration tests that spin up actual Rails projects (different versions) and do some basic smoke tests, otherwise I have to do this all manually. 🤔

💯 agree. I started going down the path of refactoring this file (largely to make it easier to test) but it felt beyond the scope of this PR. I was going to mock Webpacker in order to assert the logic, but wasn't sure how useful that would be. Mocking (obviously) doesn't test the guts of Webpacker, which is where most of the problems are likely to arise in the first place.

Integration tests would be hugely beneficial. I don't have much experience writing them though, and don't really even know where to start testing multiple Rails apps 😕 Maybe Appraisal could help with this?

In the meantime, I'd be happy to add some unit tests using mocks, if you think that would be helpful.

jamesmartin · 2020-02-12T22:53:49Z

Integration tests would be hugely beneficial. I don't have much experience writing them though, and don't really even know where to start testing multiple Rails apps 😕 Maybe Appraisal could help with this?

Thanks for the tip, I'll take a look at Appraisal. ✨

In the meantime, I'd be happy to add some unit tests using mocks, if you think that would be helpful.

I think some unit tests would be helpful to clarify our intentions, even with the problems associated with using Mocks. I would appreciate it, if you don't mind adding some. 👍

connorshea · 2020-02-13T00:11:45Z

I've generally found that using rails generator scripts can be useful, it's what we do in sorbet-rails. Unfortunately they require either committing the app(s) to the repo or generating them every time you run the test suite, neither of which is really ideal.

connorshea · 2020-02-13T00:23:37Z

Just tested it in my app and it seems to work fine, adding a new SVG works fine and everything.

kylefox · 2020-02-13T01:00:29Z

Mocking Webpacker, Rails, and network requests is proving to be more complex than I anticipated 😓

You can see what I've got so far — it's ugly: kylefox@17ca964

Let me know if you want me to continue down this path.

It may be better to introduce tests (including integration tests) against Webpacker/Rails directly in a new pull request. However, I probably won't have time to tackle this myself. 😕

jamesmartin · 2020-02-13T23:37:51Z

You can see what I've got so far — it's ugly: kylefox@17ca964

Let me know if you want me to continue down this path.

Yeah, I see what you mean. Do you feel like these tests are worth having or would just be brittle overhead? 🤔

It may be better to introduce tests (including integration tests) against Webpacker/Rails directly in a new pull request. However, I probably won't have time to tackle this myself. 😕

I started playing with this idea in a separate repo that I maintain to manually test the gem on different versions of Rails. You can see that the integration tests I was able to write are fairly comprehensive and I was able to get the tests running on rails3, rails4, rails5 and rails6 with Sprockets. The tests for the rails6-webpacker branch are currently broken because of a configuration problem but I think they should be fixable.

I'm wondering if we could use the integration tests for my test app as the integration tests for this gem somehow. I'll think more about how that might work. 🤔

jamesmartin · 2020-02-14T19:53:19Z

The tests for the rails6-webpacker branch is working now, so I feel pretty good about the test coverage in that repo.

@kylefox I think we could probably do without the brittle unit tests for this code if you're happy with it.

jamesmartin · 2020-02-18T04:21:11Z

I've setup integration tests against five different Rails versions. The workflow is kind of gnarly and there's an example of them all passing.

@kylefox if you update this branch with the latest changes from master the tests should run against your changes.

…e methods

kylefox · 2020-02-22T23:24:50Z

@jamesmartin Cool, I just rebased this branch against upstream/master. I would be happy to skip the mock unit tests and rely on these integration tests 👍

jamesmartin · 2020-02-23T21:34:23Z

Thanks again for your work on this, @kylefox. ✨ I've merged your PR and will hopefully make a new patch release in the next week or so.

kylefox · 2020-02-23T23:58:14Z

Awesome, thanks so much!

kylefox · 2020-03-17T01:50:07Z

Hey @jamesmartin, just wanted to bump this 🙂 Would it be possible to release a patch version with this fix?

jamesmartin · 2020-03-17T04:11:06Z

@kylefox apologies, I actually released this as part of 1.7.0 but forgot to add it to the release notes. It is in the CHANGELOG though. 🤦‍♂

kylefox commented Feb 12, 2020

View reviewed changes

kylefox requested a review from jamesmartin February 12, 2020 21:36

jamesmartin reviewed Feb 12, 2020

View reviewed changes

kylefox force-pushed the ssl-webpack-dev-server branch from 24d27e0 to 9940431 Compare February 13, 2020 17:37

kylefox added 4 commits February 22, 2020 16:22

Support SSL with webpack dev server

78f7bf7

break network request and tempfile construction into different privat…

0477fec

…e methods

better error handling/messages when fetching from webpack-dev-server

56989a8

make WebpackAssetFinder#pathname do less

d232345

kylefox force-pushed the ssl-webpack-dev-server branch from 9940431 to d232345 Compare February 22, 2020 23:22

jamesmartin approved these changes Feb 23, 2020

View reviewed changes

jamesmartin merged commit 4ed2974 into jamesmartin:master Feb 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix "EOFError error" when using webpack dev server over HTTPS #113

Fix "EOFError error" when using webpack dev server over HTTPS #113

kylefox commented Feb 12, 2020 •

edited

Loading

kylefox Feb 12, 2020

kylefox commented Feb 12, 2020

kylefox Feb 12, 2020

jamesmartin left a comment

kylefox commented Feb 12, 2020

jamesmartin commented Feb 12, 2020

connorshea commented Feb 13, 2020

connorshea commented Feb 13, 2020

kylefox commented Feb 13, 2020

jamesmartin commented Feb 13, 2020 •

edited

Loading

jamesmartin commented Feb 14, 2020 •

edited

Loading

jamesmartin commented Feb 18, 2020

kylefox commented Feb 22, 2020

jamesmartin commented Feb 23, 2020

kylefox commented Feb 23, 2020

kylefox commented Mar 17, 2020

jamesmartin commented Mar 17, 2020

Fix "EOFError error" when using webpack dev server over HTTPS #113

Fix "EOFError error" when using webpack dev server over HTTPS #113

Conversation

kylefox commented Feb 12, 2020 • edited Loading

kylefox Feb 12, 2020

Choose a reason for hiding this comment

kylefox commented Feb 12, 2020

kylefox Feb 12, 2020

Choose a reason for hiding this comment

jamesmartin left a comment

Choose a reason for hiding this comment

kylefox commented Feb 12, 2020

jamesmartin commented Feb 12, 2020

connorshea commented Feb 13, 2020

connorshea commented Feb 13, 2020

kylefox commented Feb 13, 2020

jamesmartin commented Feb 13, 2020 • edited Loading

jamesmartin commented Feb 14, 2020 • edited Loading

jamesmartin commented Feb 18, 2020

kylefox commented Feb 22, 2020

jamesmartin commented Feb 23, 2020

kylefox commented Feb 23, 2020

kylefox commented Mar 17, 2020

jamesmartin commented Mar 17, 2020

kylefox commented Feb 12, 2020 •

edited

Loading

jamesmartin commented Feb 13, 2020 •

edited

Loading

jamesmartin commented Feb 14, 2020 •

edited

Loading