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

Error reporting in production #2686

Closed
matthewwithanm opened this Issue Dec 9, 2014 · 69 comments

Comments

Projects
None yet
@matthewwithanm
Member

matthewwithanm commented Dec 9, 2014

We've been using Sentry to log errors in our server-side rendering and client-side. Unfortunately, the error message that React throws in production mode is pretty useless, so we wind up with a lot of unactionable reporting.

React has two levels of errors—warnings (those logged to the console) and errors (the ones that are thrown)—but both are enabled/disabled using the same env var/compile option. It would be really great if there were two different options so that I could reap the benefits of disabling warnings (e.g. from runtime type checking) without losing meaningful error messages (for example, from checksum violations).

Thanks!

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Dec 15, 2014

Member

+1, I also have lots of these “minified” exceptions in production. Stack traces help, but they're not always captured reliably.

Member

gaearon commented Dec 15, 2014

+1, I also have lots of these “minified” exceptions in production. Stack traces help, but they're not always captured reliably.

@PrototypeAlex

This comment has been minimized.

Show comment
Hide comment
@PrototypeAlex

PrototypeAlex Jan 22, 2015

We've also hit this problem, however our stacktraces give no clue as to what's happened.

👍 for meaningful error messages in production.

PrototypeAlex commented Jan 22, 2015

We've also hit this problem, however our stacktraces give no clue as to what's happened.

👍 for meaningful error messages in production.

@why-jay

This comment has been minimized.

Show comment
Hide comment
@why-jay

why-jay Aug 2, 2015

+1, wondering if there have been any updates here

why-jay commented Aug 2, 2015

+1, wondering if there have been any updates here

@mathieumg

This comment has been minimized.

Show comment
Hide comment
@mathieumg

mathieumg Aug 12, 2015

Contributor

+1 👍

Contributor

mathieumg commented Aug 12, 2015

+1 👍

@Rob-ot

This comment has been minimized.

Show comment
Hide comment
@Rob-ot

Rob-ot Sep 2, 2015

I don't know how feasible this is but it'd be nice to be able to have full error messages in production without performance measurements and other things that slow down the app.

Rob-ot commented Sep 2, 2015

I don't know how feasible this is but it'd be nice to be able to have full error messages in production without performance measurements and other things that slow down the app.

@Rob-ot

This comment has been minimized.

Show comment
Hide comment
@Rob-ot

Rob-ot Nov 30, 2015

For 0.12 I hacked a solution where I replaced some of the if ("production" !== process.env.NODE_ENV) { checks with my own environment variable. It worked pretty well.

If I pullrequest a similar approach to 0.15 would that be the right direction for it to be accepted?

I'm thinking I'd replace some (not all) of the checks with something like

if ("production" !== process.env.NODE_ENV && !process.env.REACT_PRODUCTION_DEBUGGING) {

so you could set REACT_PRODUCTION_DEBUGGING and NODE_ENV=production to do a production build with error reporting enabled. This will still allow minifiers to optimize out the unused code. Is that best way to approach this?

Rob-ot commented Nov 30, 2015

For 0.12 I hacked a solution where I replaced some of the if ("production" !== process.env.NODE_ENV) { checks with my own environment variable. It worked pretty well.

If I pullrequest a similar approach to 0.15 would that be the right direction for it to be accepted?

I'm thinking I'd replace some (not all) of the checks with something like

if ("production" !== process.env.NODE_ENV && !process.env.REACT_PRODUCTION_DEBUGGING) {

so you could set REACT_PRODUCTION_DEBUGGING and NODE_ENV=production to do a production build with error reporting enabled. This will still allow minifiers to optimize out the unused code. Is that best way to approach this?

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits Dec 1, 2015

Member

I would sooner have a REACT_ENV or something that takes precedence over NODE_ENV, but changing how this works will cause a lot of pain since everyone would need to update their build processes so I don't want to do it lightly.

Member

sophiebits commented Dec 1, 2015

I would sooner have a REACT_ENV or something that takes precedence over NODE_ENV, but changing how this works will cause a lot of pain since everyone would need to update their build processes so I don't want to do it lightly.

@syranide

This comment has been minimized.

Show comment
Hide comment
@syranide

syranide Dec 1, 2015

Contributor

@spicyj On another note, could it not make sense to have IDs associated with errors and keep those in the minified builds along with outputting the additional arguments provided, that way you can always match up the message and values later without significantly impacting bundle size.

Contributor

syranide commented Dec 1, 2015

@spicyj On another note, could it not make sense to have IDs associated with errors and keep those in the minified builds along with outputting the additional arguments provided, that way you can always match up the message and values later without significantly impacting bundle size.

@Rob-ot

This comment has been minimized.

Show comment
Hide comment
@Rob-ot

Rob-ot Dec 1, 2015

@spicyj Maybe we'd want to use REACT_ENV if it exists and fallback to NODE_ENV for backwards compat? Then REACT_ENV would be one of 3 options: production, production-with-debugging, or anything else for dev mode?

Maybe REACT_ENV is a bad name since it's more of a build mode than an environment. The important bit is in production-with-debugging mode it has usable error messages but none of the slow type checking and assertions. You get the idea though.

Rob-ot commented Dec 1, 2015

@spicyj Maybe we'd want to use REACT_ENV if it exists and fallback to NODE_ENV for backwards compat? Then REACT_ENV would be one of 3 options: production, production-with-debugging, or anything else for dev mode?

Maybe REACT_ENV is a bad name since it's more of a build mode than an environment. The important bit is in production-with-debugging mode it has usable error messages but none of the slow type checking and assertions. You get the idea though.

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits Dec 1, 2015

Member

@syranide Yeah, I do want to do this. It's probably the best solution here.

Member

sophiebits commented Dec 1, 2015

@syranide Yeah, I do want to do this. It's probably the best solution here.

@PrototypeAlex

This comment has been minimized.

Show comment
Hide comment
@PrototypeAlex

PrototypeAlex Dec 1, 2015

I agree with @Rob-ot about the fallback for backwards compatibility.

Though I'm not a fan of flipping feature switches based of broad ENV types, I'd rather see it as an isolated ENV variable with a meaningful name, you suggested REACT_PRODUCTION_DEBUGGING for example, as apposed to overloading REACT_ENV.

But that's just me 😄, I'm actually happy with anything that will enable us to switch on debugging.

PrototypeAlex commented Dec 1, 2015

I agree with @Rob-ot about the fallback for backwards compatibility.

Though I'm not a fan of flipping feature switches based of broad ENV types, I'd rather see it as an isolated ENV variable with a meaningful name, you suggested REACT_PRODUCTION_DEBUGGING for example, as apposed to overloading REACT_ENV.

But that's just me 😄, I'm actually happy with anything that will enable us to switch on debugging.

@slorber

This comment has been minimized.

Show comment
Hide comment
@slorber

slorber Dec 11, 2015

Contributor

Maybe we need a different env variable for each feature instead of having an env variable then: we should allow fine grained control over what is enabled/disabled.
The NODE_ENV could be used to compute a meaningful default for all features, and be compatible with existing build systems.

Like:

var isProduction = process.NODE_ENV === "production";

var perfMeasuresDefault = !isProduction;

var isPerfMeasures = process.REACTJS_PERF_MEASURES ? process.REACTJS_PERF_MEASURES === "true" : perfMeasuresDefault

It could be nice to have a little guide on how each of these features could impact performance so that we can decide if we want them or not.

However I think having meaningful errors in production should be a default setting. I don't understand who would be interested to have little perf/bundle size improvements over useless error messages.

I constantly got useless bug reports like this one:

comment bug

The user says he can't delete his own comment (but actually it can come from anywhere because there was probably an error thrown in render())

The user is kind enough to give a screenshot of the console error, however the error is useless and I have no way to make it more useful.
I also use GetSentry and once React starts to throw an error in a render method is generates a lot of errors afterward and make a lot of error reportings in GetSentry.

I guess it is related to catching errors in render() too, and some existing issues like #5528

In a general way it would be nice to have some doc on how to run React for production (I mean, more than just setting the NODE_ENV flag). I'm talking about topics like:

  • Ease React production bug reporting
  • Don't make the UI totally unusable after some errors are thrown
  • Don't generate 10000 useless errors if the UI is in a really bad state, and eventually give opportunity to ask the user to refresh the page or something
Contributor

slorber commented Dec 11, 2015

Maybe we need a different env variable for each feature instead of having an env variable then: we should allow fine grained control over what is enabled/disabled.
The NODE_ENV could be used to compute a meaningful default for all features, and be compatible with existing build systems.

Like:

var isProduction = process.NODE_ENV === "production";

var perfMeasuresDefault = !isProduction;

var isPerfMeasures = process.REACTJS_PERF_MEASURES ? process.REACTJS_PERF_MEASURES === "true" : perfMeasuresDefault

It could be nice to have a little guide on how each of these features could impact performance so that we can decide if we want them or not.

However I think having meaningful errors in production should be a default setting. I don't understand who would be interested to have little perf/bundle size improvements over useless error messages.

I constantly got useless bug reports like this one:

comment bug

The user says he can't delete his own comment (but actually it can come from anywhere because there was probably an error thrown in render())

The user is kind enough to give a screenshot of the console error, however the error is useless and I have no way to make it more useful.
I also use GetSentry and once React starts to throw an error in a render method is generates a lot of errors afterward and make a lot of error reportings in GetSentry.

I guess it is related to catching errors in render() too, and some existing issues like #5528

In a general way it would be nice to have some doc on how to run React for production (I mean, more than just setting the NODE_ENV flag). I'm talking about topics like:

  • Ease React production bug reporting
  • Don't make the UI totally unusable after some errors are thrown
  • Don't generate 10000 useless errors if the UI is in a really bad state, and eventually give opportunity to ask the user to refresh the page or something
@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits Dec 11, 2015

Member

I also use GetSentry and once React starts to throw an error in a render method is generates a lot of errors afterward and make a lot of error reportings in GetSentry.

I guess it is related to catching errors in render() too, and some existing issues like #5528

Yes, this is #2461 which is a large project but obviously important.

Member

sophiebits commented Dec 11, 2015

I also use GetSentry and once React starts to throw an error in a render method is generates a lot of errors afterward and make a lot of error reportings in GetSentry.

I guess it is related to catching errors in render() too, and some existing issues like #5528

Yes, this is #2461 which is a large project but obviously important.

@dcramer

This comment has been minimized.

Show comment
Hide comment
@dcramer

dcramer Apr 12, 2016

fwiw I can speak for many other Sentry users who this is also affecting, including the Sentry team itself who uses React. Let us know what we can do and we're happy to help make this happen.

dcramer commented Apr 12, 2016

fwiw I can speak for many other Sentry users who this is also affecting, including the Sentry team itself who uses React. Let us know what we can do and we're happy to help make this happen.

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Apr 13, 2016

Member

As a summary we should try to solve something for this while also optimizing for constraints:

  1. React is big to download/parse!
  2. The React ecosystem has too many options/configurations to learn!
  3. React is too slow!
  4. Error messages are too short and cryptic!
  5. Different tools doesn't work with different combinations of global configurations/compliers.

Solving one issue at a time doesn't work for constraint solving.  

Anyway. This came up recently again. I think it was in a React Native issue. The solution I suggested there was that we'd compile to error codes and put them in a lookup somehow. Like @syranide and @spicyj suggested above.

This is what we do for stack traces. Minified and compiled code often doesn't have legible stack traces or even the stack frames at all for the debugging. Source maps reinsert them so that's what we should do as well.

This seems like a much more general problem though. This is something the ecosystem needs to address, just like what to do about console.error calls from our warnings. I don't think the React repo itself should be on the hook for solving general JS problems. Maybe React should just spit out throw new Error('...') and stripping the error message + generating a source map + error code lookup file is left up to your minifier/compiler. Someone should build this solution for Babel and then we'll adopt it.

Member

sebmarkbage commented Apr 13, 2016

As a summary we should try to solve something for this while also optimizing for constraints:

  1. React is big to download/parse!
  2. The React ecosystem has too many options/configurations to learn!
  3. React is too slow!
  4. Error messages are too short and cryptic!
  5. Different tools doesn't work with different combinations of global configurations/compliers.

Solving one issue at a time doesn't work for constraint solving.  

Anyway. This came up recently again. I think it was in a React Native issue. The solution I suggested there was that we'd compile to error codes and put them in a lookup somehow. Like @syranide and @spicyj suggested above.

This is what we do for stack traces. Minified and compiled code often doesn't have legible stack traces or even the stack frames at all for the debugging. Source maps reinsert them so that's what we should do as well.

This seems like a much more general problem though. This is something the ecosystem needs to address, just like what to do about console.error calls from our warnings. I don't think the React repo itself should be on the hook for solving general JS problems. Maybe React should just spit out throw new Error('...') and stripping the error message + generating a source map + error code lookup file is left up to your minifier/compiler. Someone should build this solution for Babel and then we'll adopt it.

@dcramer

This comment has been minimized.

Show comment
Hide comment
@dcramer

dcramer Apr 13, 2016

What I'd like to see is JavaScript adopt standard exceptions. Is there an actual concern with generating consistent error messages? I don't think solutions like what Angular does (embedding URLs and trying to add debug info) are needed, but rather we need to treat JavaScript like every other programming language. 
Tools like Sentry can be responsible for parsing and making sense of errors at a higher level, but they can only do that if the libraries and frameworks don't obscure details. 
I don't know the internals of React, but what is the concern with just ensuring that the errors, at the very least, throw standard exceptions?

dcramer commented Apr 13, 2016

What I'd like to see is JavaScript adopt standard exceptions. Is there an actual concern with generating consistent error messages? I don't think solutions like what Angular does (embedding URLs and trying to add debug info) are needed, but rather we need to treat JavaScript like every other programming language. 
Tools like Sentry can be responsible for parsing and making sense of errors at a higher level, but they can only do that if the libraries and frameworks don't obscure details. 
I don't know the internals of React, but what is the concern with just ensuring that the errors, at the very least, throw standard exceptions?

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Apr 13, 2016

Member

Note that preserving error messages is not a very standard feature of programming languages in general. Many native application just provide very small and coarse grained symbols.

Systems like C# or Java that preserve them don't have to deal with the problem of download size and memory usage.

Error messages at scale add a lot of bytes to an application and stripping them out is a significant win. You could imagine a compromise with shorter messages, but if you need to look up what it means, you might as well just go all the way and add an error code.

Right now the relative size is pretty small but we have plans on focusing on minimizing the byte size of the React package because it has been of some concern to people. At that point, these messages will represent a significantly larger relative size that it will start making an impact. When you expand the problem to Relay and React Native it becomes more noticable.

Of course, we could put this burden on the end user of our library. E.g. if you don't have a compiler for your website, then you don't get it and your users suffer for it. That doesn't help for CDN usage and we're also not being very good citizens of the community. Many sites don't have very advanced polyfill and compiler solutions. To some degree, we cater to a lower common denominator so we want to provide a decent solution for existing tooling.

Similarly, we do include an Object.assign polyfill by default even though we could just require you to have a polyfill. There isn't really good standard solution for global polyfills in Node for example. So we just do it ourselves so that most people get a decent production set up by default.

I'd be happy to have our transform spit out some unique error code for each message that goes into a json file for reference though. That way you could restore the error messages.

Note that our thrown errors are not super helpful anyway. Less useful than the warnings. I'd be curious to see what kind of errors you get and if it is much more helpful.

Member

sebmarkbage commented Apr 13, 2016

Note that preserving error messages is not a very standard feature of programming languages in general. Many native application just provide very small and coarse grained symbols.

Systems like C# or Java that preserve them don't have to deal with the problem of download size and memory usage.

Error messages at scale add a lot of bytes to an application and stripping them out is a significant win. You could imagine a compromise with shorter messages, but if you need to look up what it means, you might as well just go all the way and add an error code.

Right now the relative size is pretty small but we have plans on focusing on minimizing the byte size of the React package because it has been of some concern to people. At that point, these messages will represent a significantly larger relative size that it will start making an impact. When you expand the problem to Relay and React Native it becomes more noticable.

Of course, we could put this burden on the end user of our library. E.g. if you don't have a compiler for your website, then you don't get it and your users suffer for it. That doesn't help for CDN usage and we're also not being very good citizens of the community. Many sites don't have very advanced polyfill and compiler solutions. To some degree, we cater to a lower common denominator so we want to provide a decent solution for existing tooling.

Similarly, we do include an Object.assign polyfill by default even though we could just require you to have a polyfill. There isn't really good standard solution for global polyfills in Node for example. So we just do it ourselves so that most people get a decent production set up by default.

I'd be happy to have our transform spit out some unique error code for each message that goes into a json file for reference though. That way you could restore the error messages.

Note that our thrown errors are not super helpful anyway. Less useful than the warnings. I'd be curious to see what kind of errors you get and if it is much more helpful.

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Apr 13, 2016

Member

I think the bigger issue is that we intentionally remove a lot of our thrown errors all together because the test adds execution time to hot paths.

Since we have so few errors left, maybe it doesn't matter anymore, but it also doesn't solve the issue.

What you really want is the ability to add fine grained error tracking for a subset of users that include some of the warnings. Our new devtools plugin system will hopefully let you do that at least for "profiling" builds of React.

Member

sebmarkbage commented Apr 13, 2016

I think the bigger issue is that we intentionally remove a lot of our thrown errors all together because the test adds execution time to hot paths.

Since we have so few errors left, maybe it doesn't matter anymore, but it also doesn't solve the issue.

What you really want is the ability to add fine grained error tracking for a subset of users that include some of the warnings. Our new devtools plugin system will hopefully let you do that at least for "profiling" builds of React.

@mitsuhiko

This comment has been minimized.

Show comment
Hide comment
@mitsuhiko

mitsuhiko Apr 13, 2016

@sebmarkbage

Note that preserving error messages is not a very standard feature of programming languages in general. Many native application just provide very small and coarse grained symbols.

This typically only affects symbols and not actual error messages. The only thing that gets removed in production builds are typically asserts that go away. Error messages are retained as often they show up in the console or elsewhere. However for the rare case that an error does get removed, often the function the error is raised from is named in such a way that it becomes obvious from the stacktrace what happened. As an example on iOS core foundation will execute code from __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ which shows up pretty clear in stacktraces.

JavaScript in theory has sourcemaps however they only map locations, not symbol names so that would mean if that function name gets minimized it's not really helping.

mitsuhiko commented Apr 13, 2016

@sebmarkbage

Note that preserving error messages is not a very standard feature of programming languages in general. Many native application just provide very small and coarse grained symbols.

This typically only affects symbols and not actual error messages. The only thing that gets removed in production builds are typically asserts that go away. Error messages are retained as often they show up in the console or elsewhere. However for the rare case that an error does get removed, often the function the error is raised from is named in such a way that it becomes obvious from the stacktrace what happened. As an example on iOS core foundation will execute code from __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ which shows up pretty clear in stacktraces.

JavaScript in theory has sourcemaps however they only map locations, not symbol names so that would mean if that function name gets minimized it's not really helping.

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Apr 13, 2016

Member

That's generally just for dynamically linked, right? For statically linked libraries like is common on iOS, you optimize those symbols unless you have the debug symbols locally which is similar to a source map.

Source maps does include references to source code and the ability to expand a stack trace to more frames, e.g. when a function was inlined. So you can reproduce the stack trace. React Native's error messages does this by default for example.

Btw, we don't do anything to change execution flow between modes. We've had a lot of production bugs due to such strategies in the past (both JS and native). An example is that you do a network request or mutation right after an assertion. If it gets aborted in development mode it might look like a small bug. You'll never know how bad that crash will be when it goes into production when the network request or mutation continues along. We've had pretty bad bugs due to this so we've made it very intentional to never change execution flow.

This means that we don't strip out assertions that would crash or throw in development. Similarly, because we want to be able to strip out the more expensive assertions, we turn them into logs instead of throws.

Member

sebmarkbage commented Apr 13, 2016

That's generally just for dynamically linked, right? For statically linked libraries like is common on iOS, you optimize those symbols unless you have the debug symbols locally which is similar to a source map.

Source maps does include references to source code and the ability to expand a stack trace to more frames, e.g. when a function was inlined. So you can reproduce the stack trace. React Native's error messages does this by default for example.

Btw, we don't do anything to change execution flow between modes. We've had a lot of production bugs due to such strategies in the past (both JS and native). An example is that you do a network request or mutation right after an assertion. If it gets aborted in development mode it might look like a small bug. You'll never know how bad that crash will be when it goes into production when the network request or mutation continues along. We've had pretty bad bugs due to this so we've made it very intentional to never change execution flow.

This means that we don't strip out assertions that would crash or throw in development. Similarly, because we want to be able to strip out the more expensive assertions, we turn them into logs instead of throws.

@mitsuhiko

This comment has been minimized.

Show comment
Hide comment
@mitsuhiko

mitsuhiko Apr 13, 2016

That's generally just for dynamically linked, right? For statically linked libraries like is common on iOS, you optimize those symbols unless you have the debug symbols locally which is similar to a source map.

I think we should make a distinction between symbols and error messages here. Symbols are a solved problem in both native and javascript with debug symbols/pdb files and sourcemaps respectively.

If hypothetically such optimized exceptions would carry an identifier that can be used to look up some useful information that would already go ways. This i what HRESULT on windows does in many ways. It's like an errno on steroids that can be resolved to quite appropriate error messages without creating binary bloat.

mitsuhiko commented Apr 13, 2016

That's generally just for dynamically linked, right? For statically linked libraries like is common on iOS, you optimize those symbols unless you have the debug symbols locally which is similar to a source map.

I think we should make a distinction between symbols and error messages here. Symbols are a solved problem in both native and javascript with debug symbols/pdb files and sourcemaps respectively.

If hypothetically such optimized exceptions would carry an identifier that can be used to look up some useful information that would already go ways. This i what HRESULT on windows does in many ways. It's like an errno on steroids that can be resolved to quite appropriate error messages without creating binary bloat.

@jedwards1211

This comment has been minimized.

Show comment
Hide comment
@jedwards1211

jedwards1211 Apr 14, 2016

Contributor

@sebmarkbage I completely understand and agree with you that unminified errors in a real live production app are a bad idea. However, they would be a big help when testing/debugging the production build in a sandbox, because sometimes errors occur in production that don't occur in a development build. So I think an option to have unminified errors in a production build, with documentation that explains all the drawbacks of using it for anything other than testing/debugging, would be the biggest win for developers.

Contributor

jedwards1211 commented Apr 14, 2016

@sebmarkbage I completely understand and agree with you that unminified errors in a real live production app are a bad idea. However, they would be a big help when testing/debugging the production build in a sandbox, because sometimes errors occur in production that don't occur in a development build. So I think an option to have unminified errors in a production build, with documentation that explains all the drawbacks of using it for anything other than testing/debugging, would be the biggest win for developers.

@jimfb

This comment has been minimized.

Show comment
Hide comment
@jimfb

jimfb Apr 14, 2016

Contributor

@jedwards1211 do you have an example of an error (non-contrived) that only occurred in production? While it is conceivable that such a thing could happen, it should be extraordinary uncommon in practice. If it does happen, we'd like to know asap.

Contributor

jimfb commented Apr 14, 2016

@jedwards1211 do you have an example of an error (non-contrived) that only occurred in production? While it is conceivable that such a thing could happen, it should be extraordinary uncommon in practice. If it does happen, we'd like to know asap.

@jedwards1211

This comment has been minimized.

Show comment
Hide comment
@jedwards1211

jedwards1211 Apr 14, 2016

Contributor

@jimfb it definitely wasn't an inconsistency between the development and production builds of React; it was a race condition in my own code. The difference in speed and load time of the development and production builds of course can affect race conditions.

Unfortunately I don't remember the exact details, but I know it was because I forgot to wrap my ReactDOM.render call in Meteor.startup(() => {...}) (I'm using Meteor). Somehow in dev mode it managed to always run after Meteor was ready anyway, but in production it ran too soon, and tried to access some Meteor feature that wasn't yet loaded, throwing an exception. I don't remember exactly how the error got caught and minified by React though.

So unless it should be impossible for userland race conditions to throw errors that would get caught and minified by React (and I would think it's always possible, for instance a component's render accidentally returns undefined because the programmer mistakenly assumed certain async-loaded data would be present by the time it renders, but it's not), the possibility of minified errors only in production exists.

Contributor

jedwards1211 commented Apr 14, 2016

@jimfb it definitely wasn't an inconsistency between the development and production builds of React; it was a race condition in my own code. The difference in speed and load time of the development and production builds of course can affect race conditions.

Unfortunately I don't remember the exact details, but I know it was because I forgot to wrap my ReactDOM.render call in Meteor.startup(() => {...}) (I'm using Meteor). Somehow in dev mode it managed to always run after Meteor was ready anyway, but in production it ran too soon, and tried to access some Meteor feature that wasn't yet loaded, throwing an exception. I don't remember exactly how the error got caught and minified by React though.

So unless it should be impossible for userland race conditions to throw errors that would get caught and minified by React (and I would think it's always possible, for instance a component's render accidentally returns undefined because the programmer mistakenly assumed certain async-loaded data would be present by the time it renders, but it's not), the possibility of minified errors only in production exists.

@jedwards1211

This comment has been minimized.

Show comment
Hide comment
@jedwards1211

jedwards1211 Apr 14, 2016

Contributor

render returning undefined is not the best example, since programmers should guarantee it returns an element, it was just the only way to generate a React error I could think of off the top of my head. I'm sure there are other ways.

Contributor

jedwards1211 commented Apr 14, 2016

render returning undefined is not the best example, since programmers should guarantee it returns an element, it was just the only way to generate a React error I could think of off the top of my head. I'm sure there are other ways.

@jimfb

This comment has been minimized.

Show comment
Hide comment
@jimfb

jimfb Apr 14, 2016

Contributor

@jedwards1211 The tricky thing about race conditions is that, in a build with better error messages (and potentially some warnings), you're still going to have a performance impact. The main differences in performance are a result of us eliminating all the debugging information and logic. If you add that info back into the production build, the performance characteristics will likely resemble those of the dev build.

to throw errors that would get caught and minified by React

React won't catch/minify userland code/traces. If your userland code is minified, that's some other tool in your tool chain. If you just want an unminified production build of React, you can build dev with NODE_ENV=production, which would get you what you want (an unminified production build).

Contributor

jimfb commented Apr 14, 2016

@jedwards1211 The tricky thing about race conditions is that, in a build with better error messages (and potentially some warnings), you're still going to have a performance impact. The main differences in performance are a result of us eliminating all the debugging information and logic. If you add that info back into the production build, the performance characteristics will likely resemble those of the dev build.

to throw errors that would get caught and minified by React

React won't catch/minify userland code/traces. If your userland code is minified, that's some other tool in your tool chain. If you just want an unminified production build of React, you can build dev with NODE_ENV=production, which would get you what you want (an unminified production build).

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Apr 14, 2016

Member

@jedwards1211 I think the example you mention requires a middle ground build. Something that have most of the expensive checks removed so that the race condition gets hit in time. But still with nice debug messages.

The problem with this is that these different options create an explosion of different options. You could conceive of a configuration which would catch this scenario but not another so which configuration do you use? Then we have an explosion of best practices that all depend on whether someone hit a particular scenario involved. I think it configuration like this needs to have a very high bar.

Luckily there has been talk of a PROFILE configuration. A build that is set up to run at near full speed while retaining a lot more debug information for profiling purposes. It is designed to be used on development machines. Perhaps your scenario would've been satisfied by that option as well.

Member

sebmarkbage commented Apr 14, 2016

@jedwards1211 I think the example you mention requires a middle ground build. Something that have most of the expensive checks removed so that the race condition gets hit in time. But still with nice debug messages.

The problem with this is that these different options create an explosion of different options. You could conceive of a configuration which would catch this scenario but not another so which configuration do you use? Then we have an explosion of best practices that all depend on whether someone hit a particular scenario involved. I think it configuration like this needs to have a very high bar.

Luckily there has been talk of a PROFILE configuration. A build that is set up to run at near full speed while retaining a lot more debug information for profiling purposes. It is designed to be used on development machines. Perhaps your scenario would've been satisfied by that option as well.

@jedwards1211

This comment has been minimized.

Show comment
Hide comment
@jedwards1211

jedwards1211 Apr 15, 2016

Contributor

@jimfb you know, now that I think about it, I probably was dealing with an error minified by my own toolchain. It was over a year ago...I should really keep a log of tricky issues I've dealt with. The marginal possibility of userland code performing invalid React operations as a result of a race condition still exists, but it does seem way less likely than userland code throwing TypeErrors.

Another thing that could help catch race conditions with a dev build is babel-plugin-transform-react-remove-prop-types (I assume that propTypes checks have the biggest performance penalty of any dev-specific feature?)

@sebmarkbage in any case the profile configuration sounds good to me, and if it failed to catch a scenario like this, I don't think I would be clamoring for more options, because at that point it would be hard to predict what would even have an effect on the race condition.

Contributor

jedwards1211 commented Apr 15, 2016

@jimfb you know, now that I think about it, I probably was dealing with an error minified by my own toolchain. It was over a year ago...I should really keep a log of tricky issues I've dealt with. The marginal possibility of userland code performing invalid React operations as a result of a race condition still exists, but it does seem way less likely than userland code throwing TypeErrors.

Another thing that could help catch race conditions with a dev build is babel-plugin-transform-react-remove-prop-types (I assume that propTypes checks have the biggest performance penalty of any dev-specific feature?)

@sebmarkbage in any case the profile configuration sounds good to me, and if it failed to catch a scenario like this, I don't think I would be clamoring for more options, because at that point it would be hard to predict what would even have an effect on the race condition.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon May 25, 2016

Member

@keyanzhang started working on an error code system in #6874.
Check it out and let us know what you think about the plan!

Member

gaearon commented May 25, 2016

@keyanzhang started working on an error code system in #6874.
Check it out and let us know what you think about the plan!

@dgoldstein0

This comment has been minimized.

Show comment
Hide comment
@dgoldstein0

dgoldstein0 Jun 17, 2016

just to chime in here... I'm super interested to see what the outcome of this will be. Unfortunately, though, the codebase I work on is still on the ancient react .12 (we are in process of upgrading to .13, but we've avoided jsx in the past which has made this upgrade extra difficult), so for our purposes I've decided to fork and implement something related on top of .12.0. I realize I'll probably have to port this forward until we can catch up to whatever react version this change ships with - which I suspect will be 15 or 16?

Anyhow I started a fork of react 12 for my purposes. You can find it here, if you are interested: https://github.com/dropbox/react/tree/v12-dropbox. Basically my main change is to build a debug version which

  • keeps around error messages from invariants
  • keeps a few key deprecation warnings - legacy factory deprecation and transferPropsTo deprecation. Since when we upgrade, these warnings will turn into breakages.
  • code that appears to be important for keeping around displayNames for debugging

I'm not including in my debug build:

  • prop type checking and object freezing in prod, as the former is well known to be slow, and the latter has comments to that effect
  • avoiding various internal react errors / warnings, which I suspect are there more for developing on react itself.

Hopefully this provides an interesting data point for you guys to work from; we are working hard to catch up with the latest react now, and would definitely prefer not to have to maintain our own fork once we do.

dgoldstein0 commented Jun 17, 2016

just to chime in here... I'm super interested to see what the outcome of this will be. Unfortunately, though, the codebase I work on is still on the ancient react .12 (we are in process of upgrading to .13, but we've avoided jsx in the past which has made this upgrade extra difficult), so for our purposes I've decided to fork and implement something related on top of .12.0. I realize I'll probably have to port this forward until we can catch up to whatever react version this change ships with - which I suspect will be 15 or 16?

Anyhow I started a fork of react 12 for my purposes. You can find it here, if you are interested: https://github.com/dropbox/react/tree/v12-dropbox. Basically my main change is to build a debug version which

  • keeps around error messages from invariants
  • keeps a few key deprecation warnings - legacy factory deprecation and transferPropsTo deprecation. Since when we upgrade, these warnings will turn into breakages.
  • code that appears to be important for keeping around displayNames for debugging

I'm not including in my debug build:

  • prop type checking and object freezing in prod, as the former is well known to be slow, and the latter has comments to that effect
  • avoiding various internal react errors / warnings, which I suspect are there more for developing on react itself.

Hopefully this provides an interesting data point for you guys to work from; we are working hard to catch up with the latest react now, and would definitely prefer not to have to maintain our own fork once we do.

@keyanzhang

This comment has been minimized.

Show comment
Hide comment
@keyanzhang

keyanzhang Jun 17, 2016

Member

@dgoldstein0 Thanks! The production error code system has been implemented in #6882, #6946, and #6948; it'll be shipped with the 15.2.0 release. I totally understand that upgrading from 0.12 could take some time so it might be possible to move the error system from the current version to your fork (the newly added parts are mostly build-time only).

Member

keyanzhang commented Jun 17, 2016

@dgoldstein0 Thanks! The production error code system has been implemented in #6882, #6946, and #6948; it'll be shipped with the 15.2.0 release. I totally understand that upgrading from 0.12 could take some time so it might be possible to move the error system from the current version to your fork (the newly added parts are mostly build-time only).

@dgoldstein0

This comment has been minimized.

Show comment
Hide comment
@dgoldstein0

dgoldstein0 Jun 18, 2016

I'll look at it, but I think my fork is probably mostly working already - just need to do some testing. But I'll keep the option in mind.

dgoldstein0 commented Jun 18, 2016

I'll look at it, but I think my fork is probably mostly working already - just need to do some testing. But I'll keep the option in mind.

@STRML

This comment has been minimized.

Show comment
Hide comment
@STRML

STRML Jun 26, 2016

Contributor

Well done - eagerly awaiting 15.2.0 so we can diagnose and squash a few heisenbugs.

Contributor

STRML commented Jun 26, 2016

Well done - eagerly awaiting 15.2.0 so we can diagnose and squash a few heisenbugs.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jul 1, 2016

Member

This is out in React 15.2.0.
Thanks to @keyanzhang!

screen shot 2016-07-01 at 21 00 23

screen shot 2016-07-01 at 21 00 35

Member

gaearon commented Jul 1, 2016

This is out in React 15.2.0.
Thanks to @keyanzhang!

screen shot 2016-07-01 at 21 00 23

screen shot 2016-07-01 at 21 00 35

@gaearon gaearon closed this Jul 1, 2016

@mitsuhiko

This comment has been minimized.

Show comment
Hide comment
@mitsuhiko

mitsuhiko Jul 1, 2016

@gaearon is the error mapping available somewhere? And what's the policy for future changes to it.

mitsuhiko commented Jul 1, 2016

@gaearon is the error mapping available somewhere? And what's the policy for future changes to it.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jul 1, 2016

Member

@mitsuhiko It’s automatically extracted at the build time. I think we plan to treat it as append-only. You can find the latest version here. Error messages link you to a page on the website where we decode them, e.g. https://facebook.github.io/react/docs/error-decoder.html?invariant=31&args[]=object%20with%20keys%20%7Bname%7D&args[]=.

Member

gaearon commented Jul 1, 2016

@mitsuhiko It’s automatically extracted at the build time. I think we plan to treat it as append-only. You can find the latest version here. Error messages link you to a page on the website where we decode them, e.g. https://facebook.github.io/react/docs/error-decoder.html?invariant=31&args[]=object%20with%20keys%20%7Bname%7D&args[]=.

@keyanzhang

This comment has been minimized.

Show comment
Hide comment
@keyanzhang

keyanzhang Jul 1, 2016

Member

@mitsuhiko here is the script that extracts errors. As @gaearon mentioned, it's append-only, which means the code that have been already there won't be changed.

Member

keyanzhang commented Jul 1, 2016

@mitsuhiko here is the script that extracts errors. As @gaearon mentioned, it's append-only, which means the code that have been already there won't be changed.

@mitsuhiko

This comment has been minimized.

Show comment
Hide comment
@mitsuhiko

mitsuhiko Jul 1, 2016

Append only is good. The not so nice thing is that there is no meta information on those exceptions that tools can access. That might be a nice thing to have in the future.

mitsuhiko commented Jul 1, 2016

Append only is good. The not so nice thing is that there is no meta information on those exceptions that tools can access. That might be a nice thing to have in the future.

@dgoldstein0

This comment has been minimized.

Show comment
Hide comment
@dgoldstein0

dgoldstein0 Jul 2, 2016

question: shouldn't the react version number be in the error urls - at least the major version? (I don't quite follow what "append only" means in the context of this thread)

dgoldstein0 commented Jul 2, 2016

question: shouldn't the react version number be in the error urls - at least the major version? (I don't quite follow what "append only" means in the context of this thread)

@keyanzhang

This comment has been minimized.

Show comment
Hide comment
@keyanzhang

keyanzhang Jul 2, 2016

Member

@dgoldstein0 The JSON file and the website will always be the latest release version. "Append-only" means we won't modify or delete an existing error mapping, and a new error (or an existing error that gets newly edited) will be appended to the end of the file and get a new number. Therefore we make sure that old versions are supported.

Member

keyanzhang commented Jul 2, 2016

@dgoldstein0 The JSON file and the website will always be the latest release version. "Append-only" means we won't modify or delete an existing error mapping, and a new error (or an existing error that gets newly edited) will be appended to the end of the file and get a new number. Therefore we make sure that old versions are supported.

@dgoldstein0

This comment has been minimized.

Show comment
Hide comment
@dgoldstein0

dgoldstein0 Jul 2, 2016

got it. But don't think that helps me right now - I'm looking at backporting this to react .12 and doubt that the errors are going to come out in the same order.

dgoldstein0 commented Jul 2, 2016

got it. But don't think that helps me right now - I'm looking at backporting this to react .12 and doubt that the errors are going to come out in the same order.

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits Jul 2, 2016

Member

We don't use the major version in the URLs because the page doesn't need it. We don't have plans to support React 0.12 in the official error decoder page but you could make your own page and error code map if you were very motivated. If you are doing a custom build it may be easier for you to just delete the build code that minifies the error messages.

Member

sophiebits commented Jul 2, 2016

We don't use the major version in the URLs because the page doesn't need it. We don't have plans to support React 0.12 in the official error decoder page but you could make your own page and error code map if you were very motivated. If you are doing a custom build it may be easier for you to just delete the build code that minifies the error messages.

@dgoldstein0

This comment has been minimized.

Show comment
Hide comment
@dgoldstein0

dgoldstein0 Jul 3, 2016

already did that. The problem though is that the error messages add 32kb to react when minified, which was a bit more than I was hoping. So I'm preceding with backporting it.

I completely understand that react .12 is unsupported now; just unfortunate reality that I still have to deal with it. And even when I'm done with .12, I'll probably have .13/.14 to deal with for 2-3 months before we finally can get to 15.

dgoldstein0 commented Jul 3, 2016

already did that. The problem though is that the error messages add 32kb to react when minified, which was a bit more than I was hoping. So I'm preceding with backporting it.

I completely understand that react .12 is unsupported now; just unfortunate reality that I still have to deal with it. And even when I'm done with .12, I'll probably have .13/.14 to deal with for 2-3 months before we finally can get to 15.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jul 3, 2016

Member

@dgoldstein0 Just checking—are you aware that some of the deprecations / changes can be automated with https://github.com/reactjs/react-codemod? You don’t have to update all of the code by hand.

Member

gaearon commented Jul 3, 2016

@dgoldstein0 Just checking—are you aware that some of the deprecations / changes can be automated with https://github.com/reactjs/react-codemod? You don’t have to update all of the code by hand.

@dgoldstein0

This comment has been minimized.

Show comment
Hide comment
@dgoldstein0

dgoldstein0 Jul 4, 2016

Yes I'm aware. However that doesn't help us much since almost all our code
is coffeescript, and we didn't start using jsx until recently.

On Sun, Jul 3, 2016, 3:18 PM Dan Abramov notifications@github.com wrote:

@dgoldstein0 https://github.com/dgoldstein0 Just checking—are you aware
that some of the deprecations / changes can be automated with
https://github.com/reactjs/react-codemod? You don’t have to update all of
the code by hand.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#2686 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABBndxjbkt1KGS6DX5lN2cmIq0PebkUxks5qSDUkgaJpZM4DGLA4
.

dgoldstein0 commented Jul 4, 2016

Yes I'm aware. However that doesn't help us much since almost all our code
is coffeescript, and we didn't start using jsx until recently.

On Sun, Jul 3, 2016, 3:18 PM Dan Abramov notifications@github.com wrote:

@dgoldstein0 https://github.com/dgoldstein0 Just checking—are you aware
that some of the deprecations / changes can be automated with
https://github.com/reactjs/react-codemod? You don’t have to update all of
the code by hand.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#2686 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABBndxjbkt1KGS6DX5lN2cmIq0PebkUxks5qSDUkgaJpZM4DGLA4
.

@rafayepes

This comment has been minimized.

Show comment
Hide comment
@rafayepes

rafayepes Jul 12, 2016

@keyanzhang I don't see anything indicates the react version in the error URL https://facebook.github.io/react/docs/error-decoder.html?invariant=1&args[]=Foo&args[]=Bar

How is the versioning of codes.json managed?

rafayepes commented Jul 12, 2016

@keyanzhang I don't see anything indicates the react version in the error URL https://facebook.github.io/react/docs/error-decoder.html?invariant=1&args[]=Foo&args[]=Bar

How is the versioning of codes.json managed?

@mathieumg

This comment has been minimized.

Show comment
Hide comment
@mathieumg
Contributor

mathieumg commented Jul 12, 2016

@rafayepes

This comment has been minimized.

Show comment
Hide comment
@rafayepes

rafayepes Jul 12, 2016

Oh thanks @mathieumg ! I'm totally blind 😅

@spicyj what about future versions of React? So let's say React 16 or React 17 adds/deletes/modifies some errors (totally completely normal). A user of React 15 could go to the error URL and find an error message that is not actually the error he/she had. Are you going to make sure that old error indexes are never removed/modified? How?

So looks like the approach is never modify old errors. I'm still curious how can this be ensured and how this can be scaled across multiple developers. Seems something fairly easy to forget.

Thanks! Love this feature ^^

rafayepes commented Jul 12, 2016

Oh thanks @mathieumg ! I'm totally blind 😅

@spicyj what about future versions of React? So let's say React 16 or React 17 adds/deletes/modifies some errors (totally completely normal). A user of React 15 could go to the error URL and find an error message that is not actually the error he/she had. Are you going to make sure that old error indexes are never removed/modified? How?

So looks like the approach is never modify old errors. I'm still curious how can this be ensured and how this can be scaled across multiple developers. Seems something fairly easy to forget.

Thanks! Love this feature ^^

@mathieumg

This comment has been minimized.

Show comment
Hide comment
@mathieumg

mathieumg Jul 12, 2016

Contributor

@rafayepes I thought #2686 (comment) would have answered that concern! 😛

Contributor

mathieumg commented Jul 12, 2016

@rafayepes I thought #2686 (comment) would have answered that concern! 😛

@joshkel

This comment has been minimized.

Show comment
Hide comment
@joshkel

joshkel Jul 13, 2016

@rafayepes The error list is automatically maintained via a Gulp task, so forgetting to do it isn't a concern. See bfd1531.

joshkel commented Jul 13, 2016

@rafayepes The error list is automatically maintained via a Gulp task, so forgetting to do it isn't a concern. See bfd1531.

@benvinegar

This comment has been minimized.

Show comment
Hide comment
@benvinegar

benvinegar Aug 10, 2016

As a follow-up to this, we've modified @getsentry to fetch the original message and display it in our crash reporting UI. Pull request is getsentry/sentry#3632, announcement on our blog here.

benvinegar commented Aug 10, 2016

As a follow-up to this, we've modified @getsentry to fetch the original message and display it in our crash reporting UI. Pull request is getsentry/sentry#3632, announcement on our blog here.

@fwchen

This comment has been minimized.

Show comment
Hide comment
@fwchen

fwchen Sep 19, 2017

@benvinegar
I try Sentry but it don't caught any minified error.
it need other configure?

fwchen commented Sep 19, 2017

@benvinegar
I try Sentry but it don't caught any minified error.
it need other configure?

@tushariscoolster

This comment has been minimized.

Show comment
Hide comment
@tushariscoolster

tushariscoolster Oct 4, 2018

if (process.env.NODE_ENV === 'production') {
module.exports = require('./cjs/react-dom-server.node.production.min.js');
} else {
module.exports = require('./cjs/react-dom-server.node.development.js');
}

react-dom-server.node.production.min.js suppress all the error, can we use react-dom-server.node.development.js on production and what implication it will have?

tushariscoolster commented Oct 4, 2018

if (process.env.NODE_ENV === 'production') {
module.exports = require('./cjs/react-dom-server.node.production.min.js');
} else {
module.exports = require('./cjs/react-dom-server.node.development.js');
}

react-dom-server.node.production.min.js suppress all the error, can we use react-dom-server.node.development.js on production and what implication it will have?

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Oct 4, 2018

Member

It's much slower.

Member

gaearon commented Oct 4, 2018

It's much slower.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Oct 4, 2018

Member

react-dom-server.node.production.min.js suppress all the error

It shouldn't "suppress" any error. All errors in production version of React exists in the same way, and you should get an error code in every message so you can decode them back.

Member

gaearon commented Oct 4, 2018

react-dom-server.node.production.min.js suppress all the error

It shouldn't "suppress" any error. All errors in production version of React exists in the same way, and you should get an error code in every message so you can decode them back.

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