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

Add syntax error overlay in development #744

Merged
merged 3 commits into from
Sep 25, 2016
Merged

Add syntax error overlay in development #744

merged 3 commits into from
Sep 25, 2016

Conversation

gaearon
Copy link
Contributor

@gaearon gaearon commented Sep 25, 2016

Fixes #89.
Fixes #569.
Fully fixes #263 (we don’t log anything now, and don’t try to reconnect).

Some screenshots:

screen shot 2016-09-25 at 01 23 59

screen shot 2016-09-25 at 01 24 10

@gaearon gaearon added this to the 0.6.0 milestone Sep 25, 2016
@ghost ghost added the CLA Signed label Sep 25, 2016
@gaearon
Copy link
Contributor Author

gaearon commented Sep 25, 2016

I’m bad at CSS so if you have ideas how to make it wrap code better or something, I’m up for it.

@gaearon
Copy link
Contributor Author

gaearon commented Sep 25, 2016

Travis seems to fail because of #698.

@glenjamin
Copy link

Nice work!

I think it's worth logging just the once when a disconnect happens, with a note to reload after the server is available again?

white-space: pre-wrap might do what you need in the overlay too.

@gaearon
Copy link
Contributor Author

gaearon commented Sep 25, 2016

Going to get this in. Can fix nits later. 😄

@gaearon gaearon merged commit 9cce0fb into master Sep 25, 2016
@gaearon gaearon deleted the bsod branch September 25, 2016 10:31
@goshacmd
Copy link
Contributor

Making the overlay slightly transparent also gives a sense of depth. As in, "hey, your stuff is here, but you gotta fix this first". Minor and probably subjective, but I have liked it on several projects

@gaearon
Copy link
Contributor Author

gaearon commented Sep 25, 2016

@glenjamin

👍

screen shot 2016-09-25 at 12 38 36

@goshakkk

We might make it transparent when we support hot reloading for JS, but for now, fixing a syntax error triggers a full refresh anyway, so I think making it transparent might make it annoying (“my stuff is there, why are you refreshing?”)

message = lines.join('\n');
// Internal stacks are generally useless so we strip them
message = message.replace(
/^\s*at\s.*:\d+:\d+[\s\)]*\n/gm, ''
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are node error message internationalized? PHP ones are and this would break with them

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not as far as I know. At least not yet.

}

// https://webpack.github.io/docs/hot-module-replacement.html#check
module.hot.check(/* autoApply */true, function(err, updatedModules) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inline comment for booleans ftw :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Learned this from you!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woah this is neat, good tip!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, nice one

@jefflau
Copy link

jefflau commented Sep 30, 2016

I'm trying to merge the create-react-app into a current project so I can use all the build stuff you guys use. I've got a ton of errors from es-lint and other things which i want to gradually get rid of. The app is working behind the iframe, but I'd like it to be optional to have this massive iframe overlay so I can get rid of the errors gradually. Is that possible right now?

@gaearon
Copy link
Contributor Author

gaearon commented Sep 30, 2016

@jefflau You can remove react-dev-utils/webpackHotDevClient and instead use the stock client, as described here.

@gaearon
Copy link
Contributor Author

gaearon commented Sep 30, 2016

You can't do that without ejecting, but presumably you already ejected? Otherwise I'm not sure where lint errors are coming from. We only use lint warnings in CRA lint configuration except for no-undef which we treat as an error. So normally the overlay shouldn't show up.

@jefflau
Copy link

jefflau commented Sep 30, 2016

Hey @gaearon I ejected already to do some extra webpack build config. We've got some global variables being injected by other things, so I changed the nodef to warning until I can (if I can) change it to not use globals.

Does webpackhotdevclient do the HMR for JS? I wouldn't want to remove it if it does

@gaearon
Copy link
Contributor Author

gaearon commented Sep 30, 2016

We've got some global variables being injected by other things, so I changed the nodef to warning until I can (if I can) change it to not use globals.

You can replace myGlobal with window.myGlobal if it’s intentional, and the warning will go away.

@gaearon
Copy link
Contributor Author

gaearon commented Sep 30, 2016

Does webpackhotdevclient do the HMR for JS? I wouldn't want to remove it if it does

I haven’t tried but I think it does if you already handle HMR somewhere in the code. Our CSS hot reloads because style-loader uses HMR API, so it should work.

feiqitian pushed a commit to feiqitian/create-react-app that referenced this pull request Oct 25, 2016
* Add syntax error overlay in development

* Support HMR being disabled

* Tweak CSS
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove console "spam" Improve/silence WebpackDevServer client Display an overlay for syntax errors
7 participants