Surface Bundler error to browser #1457
Conversation
|
Seems like an awesome idea, as we already have the overlay |
|
Currently I'm stuck with pretty printing output of babel-code-frame in HTML. I know that https://new.babeljs.io/repl is doing something similar (without syntax highlighting)but I'm not sure how they print the code frame. I need to look into it. Should I consolidate this code with the overlay? Overlay is manually putting together the HTML as well. Should we depend on a templating library for generating the HTML? |
|
@mohsen1 You can probably copy that code, unfortunately it’s in built-in so we cant create a shared function |
| @@ -713,7 +713,11 @@ class Bundler extends EventEmitter { | |||
|
|
|||
| async serve(port = 1234, https = false) { | |||
| this.server = await Server.serve(this, port, https); | |||
| this.bundle(); | |||
There was a problem hiding this comment.
Previously if this line was throwing an error we had a silent unhandled promise rejection
mohsen1
changed the title
mohsen1
force-pushed
the
surface-error-to-browser
branch
from
a75e44f
to
c257d39
Compare
Codecov Report
@@ Coverage Diff @@
## master #1457 +/- ##
==========================================
+ Coverage 86.41% 87.87% +1.46%
==========================================
Files 80 80
Lines 4585 4412 -173
==========================================
- Hits 3962 3877 -85
+ Misses 623 535 -88
Continue to review full report at Codecov.
|
|
@DeMoorJasper @devongovett Can you guys take a look? This is working :) |
| res.end('🚨 Build error, check the console for details.'); | ||
| let errorMesssge = '<h1>🚨 Build Error</h1>'; | ||
| if (process.env.NODE_ENV === 'production') { | ||
| errorMesssge += '<p><b>Check the console for details.</b></p>'; |
There was a problem hiding this comment.
Not sure if this is necessary, the built-in parcel server should never be used in production
There was a problem hiding this comment.
Parcel can be used as middleware in a production server, so I guess it's good to check this anyways.
There was a problem hiding this comment.
Yeah it should never happen but just in case...
| )}</div>`; | ||
| } | ||
| } | ||
| res.end( |
There was a problem hiding this comment.
Why are you using an array instead of a template string here?
There was a problem hiding this comment.
This is to make sure we're not sending extra whitespaces or not ending up with awkward code indentations. Depending on project coding style I can update.
There was a problem hiding this comment.
Ah, alright, I don't think the weird indentation matters, but that's personal taste.
I'll let someone else review this, but looks good to me
|
@DeMoorJasper do you still require changes? |
|
Looks good to me, I'm just not 100% convinced and it's a pretty large change so I'll let someone else review it |
test/server.js
Outdated
| res.setEncoding('utf8'); | ||
| let data = ''; | ||
| res.on('data', c => (data += c)); | ||
| res.on('end', () => { | ||
| if (Math.floor(res.statusCode / 100) !== 2) { |
There was a problem hiding this comment.
Does this ever respond with a code not equal to 200?
(I mean besides the error codes 404 and 500)
There was a problem hiding this comment.
Yes. My new test case has 500 response
There was a problem hiding this comment.
I meant does it ever respond with something other than a standard 200 in case of a sucessfull reply.
Basically I am asking if it wouldn't be possible to use res.statusCode !== 200, seems like a less complex comparison and slightly faster probably.
As we don't set any other codes in server I think it should be safe to use !== 200
There was a problem hiding this comment.
if you ever respond with 201 or other 2xx you'll not need to update this line. I'm open to reverting back this to comparing strictly against 200 if you feel strongly about it
|
@devongovett can you take a look? Thanks! |
Instead of sending a generic error with the 500 response, print the error message and stack trace in the response HTML payload.
Feel free to comment.