Explore encouraging users to not ship DEV mode to production #8784

Closed
addyosmani opened this Issue Jan 13, 2017 · 143 comments

Comments

Projects
None yet
@addyosmani
Contributor

addyosmani commented Jan 13, 2017

Do you want to request a feature or report a bug?
Feature

What is the current behavior?
Developers meaning to do the right thing will often accidentally ship DEV mode to production rather than PROD mode. This can have a significant impact on performance. Although DEV->PROD is a one line change, it's something React could explore encouraging.

There's great nuance here and I know that there's balance to be struck between the overall DX value this brings vs UX. Another challenge is that the change itself is trivial to make. It's unclear whether the right solution here is better defaults or stronger advocacy. Folks like @sebmarkbage have been acknowledging that this is a known issue so perhaps there's room for discussion to help improve this.

He's also noted that a switch from no warnings to DEV may require some folks to fix whole codebases which is also suboptimal. There may be an in-between solution worth talking about here however.

What is the expected behavior?

React encourages users to ship PROD mode to production rather than DEV. I would be open to a solution that is either provided at the library layer (or somehow tackled during build/bundling time by Webpack) that tries to ameliorate this.

This thread had a number of suggestions ranging from localhost detection, to alerts to injecting 'dev mode' messages to the DOM if used in a production environment. Something like this:

Alternatively, @TheLarkInn was proposing that we tried to standardize on ENV configs being required to better facilitate detection of messaging like this. It's unclear which of these would be the most realistic. There are likely other ideas React core might have around how to tackle the problem.

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

All recent versions.

This thread from @jordwalke prompted this issue. I think he also makes a fair point regarding benchmarks, but I care about how we can help folks ship the prod experience y'all have worked on optimizing to end customers in all it's glory.

@aweary

This comment has been minimized.

Show comment
Hide comment
@aweary

aweary Jan 13, 2017

Member

For reference: #8782

Member

aweary commented Jan 13, 2017

For reference: #8782

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Jan 13, 2017

Member

For context we already warn if we detect that you've minified a DEV version of React:

var testFunc = function testFn() {};
warning(
(testFunc.name || testFunc.toString()).indexOf('testFn') !== -1,
'It looks like you\'re using a minified copy of the development build ' +
'of React. When deploying React apps to production, make sure to use ' +
'the production build which skips development warnings and is faster. ' +
'See https://fb.me/react-minification for more details.'
);

As far as we can find similar heuristics to notify users, and maybe even more aggressively pop up a DOM dialog, we should.

Member

sebmarkbage commented Jan 13, 2017

For context we already warn if we detect that you've minified a DEV version of React:

var testFunc = function testFn() {};
warning(
(testFunc.name || testFunc.toString()).indexOf('testFn') !== -1,
'It looks like you\'re using a minified copy of the development build ' +
'of React. When deploying React apps to production, make sure to use ' +
'the production build which skips development warnings and is faster. ' +
'See https://fb.me/react-minification for more details.'
);

As far as we can find similar heuristics to notify users, and maybe even more aggressively pop up a DOM dialog, we should.

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Jan 13, 2017

Member

I also want to make it clear the warnings we provide can significantly improve performance if people pay attention to it. This thread explains some rationale for why it is difficult to deploy this after the fact if it is not the default.

Member

sebmarkbage commented Jan 13, 2017

I also want to make it clear the warnings we provide can significantly improve performance if people pay attention to it. This thread explains some rationale for why it is difficult to deploy this after the fact if it is not the default.

@aickin

This comment has been minimized.

Show comment
Hide comment
@aickin

aickin Jan 13, 2017

Collaborator

I'd also like to chime in with a suggestion of a single console.warn if renderToString is called in dev mode. Obviously, in most situations renderToString is called in node, where we can't alert or pop up a DOM dialog.

Unfortunately, I'd really like to be able to detect not just if NODE_ENV is set to production, but also if process.env.NODE_ENV has been compiled out. Does anyone know of a way to do that?

Collaborator

aickin commented Jan 13, 2017

I'd also like to chime in with a suggestion of a single console.warn if renderToString is called in dev mode. Obviously, in most situations renderToString is called in node, where we can't alert or pop up a DOM dialog.

Unfortunately, I'd really like to be able to detect not just if NODE_ENV is set to production, but also if process.env.NODE_ENV has been compiled out. Does anyone know of a way to do that?

@addyosmani

This comment has been minimized.

Show comment
Hide comment
@addyosmani

addyosmani Jan 13, 2017

Contributor

Thanks for the thread @sebmarkbage and I acknowledge the difficulties of deploying after the fact. I'm also appreciative of the current warnings. It appears that some developers may not check the console output of their deployed sites as often as they could. It's a good first step however.

As far as we can find similar heuristics to notify users, and maybe even more aggressively pop up a DOM dialog, we should.

I'd be grateful for improvements to the heuristics used to notify users. A more aggressive DOM dialog would go a long way in helping. It would ensure the site continued working for end-users but provides an active hint that there are low-hanging perf fruit developers can pick.

The alternative is that we find a way to fix this at a build-tool / ENV level as mentioned in the original post. That would avoid any DOM-injection being necessary.

Contributor

addyosmani commented Jan 13, 2017

Thanks for the thread @sebmarkbage and I acknowledge the difficulties of deploying after the fact. I'm also appreciative of the current warnings. It appears that some developers may not check the console output of their deployed sites as often as they could. It's a good first step however.

As far as we can find similar heuristics to notify users, and maybe even more aggressively pop up a DOM dialog, we should.

I'd be grateful for improvements to the heuristics used to notify users. A more aggressive DOM dialog would go a long way in helping. It would ensure the site continued working for end-users but provides an active hint that there are low-hanging perf fruit developers can pick.

The alternative is that we find a way to fix this at a build-tool / ENV level as mentioned in the original post. That would avoid any DOM-injection being necessary.

@aweary

This comment has been minimized.

Show comment
Hide comment
@aweary

aweary Jan 14, 2017

Member

Injecting any messaging into the DOM seems dangerous and a little too assuming. That opens up the possibility of end users getting unexpected and confusing alerts which seems unacceptable IMO.

@TheLarkInn was proposing that we tried to standardize on ENV configs being required to better facilitate detection of messaging like this

This feels like the ideal space to address this. It's a build issue and should be, if possible, addressed by the build tools.

Member

aweary commented Jan 14, 2017

Injecting any messaging into the DOM seems dangerous and a little too assuming. That opens up the possibility of end users getting unexpected and confusing alerts which seems unacceptable IMO.

@TheLarkInn was proposing that we tried to standardize on ENV configs being required to better facilitate detection of messaging like this

This feels like the ideal space to address this. It's a build issue and should be, if possible, addressed by the build tools.

@surma

This comment has been minimized.

Show comment
Hide comment
@surma

surma Jan 14, 2017

Anecdotal: Warnings in the console have been gone unseen (or ignored) in the past. I have no hard numbers here, but I think console-based warnings are not enough. I agree with @addyosmani that a DOM-based warning would go a long way.
screenshot 2017-01-13 15 49 29

surma commented Jan 14, 2017

Anecdotal: Warnings in the console have been gone unseen (or ignored) in the past. I have no hard numbers here, but I think console-based warnings are not enough. I agree with @addyosmani that a DOM-based warning would go a long way.
screenshot 2017-01-13 15 49 29

@aweary

This comment has been minimized.

Show comment
Hide comment
@aweary

aweary Jan 14, 2017

Member

@surma maybe they should use console.warn or console.error for better visibility 😉

I don't see how it would be acceptable in any scenario to inject content into an application only when it's running in production. You're making a huge assumption that the message would be caught before the application was pushed to actual users, where the message could potentially hurt UX in a major way.

Member

aweary commented Jan 14, 2017

@surma maybe they should use console.warn or console.error for better visibility 😉

I don't see how it would be acceptable in any scenario to inject content into an application only when it's running in production. You're making a huge assumption that the message would be caught before the application was pushed to actual users, where the message could potentially hurt UX in a major way.

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Jan 14, 2017

Member

Btw, we're going to be add more comprehensive error handling support in Fiber so that you can replace components that have failures (thrown errors) with custom error views. Even in the default scenario we're likely going to be very aggressive, and just remove that component from the tree if it errors so you're really going to want to have a custom error UI for your users anyway.

We might be able to trigger such an error for this warning.

Member

sebmarkbage commented Jan 14, 2017

Btw, we're going to be add more comprehensive error handling support in Fiber so that you can replace components that have failures (thrown errors) with custom error views. Even in the default scenario we're likely going to be very aggressive, and just remove that component from the tree if it errors so you're really going to want to have a custom error UI for your users anyway.

We might be able to trigger such an error for this warning.

@surma

This comment has been minimized.

Show comment
Hide comment
@surma

surma Jan 14, 2017

I honestly don’t think that console.{error,warn} over console.log would have changed anything. As I said, that story is anecdotal.

I am also not saying that showing a DOM dialog is the solution. It’s something I’d personally be comfortable with, though. If staying in dev mode has negative impact, at least users would know that something is wrong and probably starts hitting the “Help” button or something.

Bottom line: I think frameworks need to get in the developer’s face at some point. I’d love to brainstorm on this to see what steps and compromises framework authors are willing to take to prevent people from deploying in dev mode in the future.

surma commented Jan 14, 2017

I honestly don’t think that console.{error,warn} over console.log would have changed anything. As I said, that story is anecdotal.

I am also not saying that showing a DOM dialog is the solution. It’s something I’d personally be comfortable with, though. If staying in dev mode has negative impact, at least users would know that something is wrong and probably starts hitting the “Help” button or something.

Bottom line: I think frameworks need to get in the developer’s face at some point. I’d love to brainstorm on this to see what steps and compromises framework authors are willing to take to prevent people from deploying in dev mode in the future.

@acdlite

This comment has been minimized.

Show comment
Hide comment
@acdlite

acdlite Jan 14, 2017

Member

What if React just doesn't work at all unless you provide an environment, regardless of whether it's development or prod? That way there's a conscious choice being made one way or the other.

Member

acdlite commented Jan 14, 2017

What if React just doesn't work at all unless you provide an environment, regardless of whether it's development or prod? That way there's a conscious choice being made one way or the other.

@jide

This comment has been minimized.

Show comment
Hide comment
@jide

jide Jan 14, 2017

About the message injected in the DOM, it could be disabled using a global or something. No big deal IMO. If you disable it, you kind of acknowledge you know what you're doing.

Problem with a console message is that sometimes people log a lot of things, or have other warnings they ignore, and it's easy to not see that first console message past the scroll...

jide commented Jan 14, 2017

About the message injected in the DOM, it could be disabled using a global or something. No big deal IMO. If you disable it, you kind of acknowledge you know what you're doing.

Problem with a console message is that sometimes people log a lot of things, or have other warnings they ignore, and it's easy to not see that first console message past the scroll...

@jide

This comment has been minimized.

Show comment
Hide comment
@jide

jide Jan 14, 2017

With a mandatory env, inevitably boilerplates etc. will set the env var so you can just start using it I'm afraid :/

jide commented Jan 14, 2017

With a mandatory env, inevitably boilerplates etc. will set the env var so you can just start using it I'm afraid :/

@aweary

This comment has been minimized.

Show comment
Hide comment
@aweary

aweary Jan 14, 2017

Member

I honestly don’t think that console.{error,warn} over console.log would have changed anything.

Do you think its a problem with developers just not checking the console, or the console being overloaded with warnings? Could this be (partially) addressed with a more general approach regarding developer education?

I am also not saying that showing a DOM dialog is the solution. It’s something I’d personally be comfortable with, though. If staying in dev mode has negative impact, at least users would know that something is wrong and probably starts hitting the “Help” button or something.

I understand that, but I'm just saying I don't think its a solution let alone the solution. Its good that you're comfortable with it, but I think it's better to err on the side of caution and assume that most people don't want unexpected errors displayed to their users.

Bottom line: I think frameworks need to get in the developer’s face at some point. I’d love to brainstorm on this to see what steps and compromises framework authors are willing to take to prevent people from deploying in dev mode in the future.

I am 💯 for getting in the developer's face, but it's important to do it at the right place. To me, that's the build step, not production.

About the message injected in the DOM, it could be disabled using a global or something. No big deal IMO. If you disable it, you kind of acknowledge you know what you're doing.

Having it enabled by default is just as bad as not making it configurable: the default behavior could result in unexpected behavior for the end user. If anything it should be disabled by default, but then that defeats the entire purpose since developers could just fix the initial problem once they're aware of it.

Problem with a console message is that sometimes people log a lot of things, or have other warnings they ignore, and it's easy to not see that first console message past the scroll...

I totally get that, the console can get crowded and it's easy to miss stuff. But it's crowded for the exact reasons I'm arguing: it's the place the provide developers with feedback or errors. It's out of the way of the user experience, which injected messages aren't.

Member

aweary commented Jan 14, 2017

I honestly don’t think that console.{error,warn} over console.log would have changed anything.

Do you think its a problem with developers just not checking the console, or the console being overloaded with warnings? Could this be (partially) addressed with a more general approach regarding developer education?

I am also not saying that showing a DOM dialog is the solution. It’s something I’d personally be comfortable with, though. If staying in dev mode has negative impact, at least users would know that something is wrong and probably starts hitting the “Help” button or something.

I understand that, but I'm just saying I don't think its a solution let alone the solution. Its good that you're comfortable with it, but I think it's better to err on the side of caution and assume that most people don't want unexpected errors displayed to their users.

Bottom line: I think frameworks need to get in the developer’s face at some point. I’d love to brainstorm on this to see what steps and compromises framework authors are willing to take to prevent people from deploying in dev mode in the future.

I am 💯 for getting in the developer's face, but it's important to do it at the right place. To me, that's the build step, not production.

About the message injected in the DOM, it could be disabled using a global or something. No big deal IMO. If you disable it, you kind of acknowledge you know what you're doing.

Having it enabled by default is just as bad as not making it configurable: the default behavior could result in unexpected behavior for the end user. If anything it should be disabled by default, but then that defeats the entire purpose since developers could just fix the initial problem once they're aware of it.

Problem with a console message is that sometimes people log a lot of things, or have other warnings they ignore, and it's easy to not see that first console message past the scroll...

I totally get that, the console can get crowded and it's easy to miss stuff. But it's crowded for the exact reasons I'm arguing: it's the place the provide developers with feedback or errors. It's out of the way of the user experience, which injected messages aren't.

@jide

This comment has been minimized.

Show comment
Hide comment
@jide

jide Jan 14, 2017

Makes sense, I understand the reasonning.

Well maybe make the thing pop out using console formatting would be something at least.

capture d ecran 2017-01-14 a 02 51 44

jide commented Jan 14, 2017

Makes sense, I understand the reasonning.

Well maybe make the thing pop out using console formatting would be something at least.

capture d ecran 2017-01-14 a 02 51 44

@jide

This comment has been minimized.

Show comment
Hide comment

jide commented Jan 14, 2017

capture d ecran 2017-01-14 a 03 05 49

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jan 14, 2017

Member

The problem is almost nobody looks at console in production. You can use any font there and people won't notice it.

However if we make it show a message by default in production as a breaking change (in 16 or 17) then it would be hard to miss. I mean, it would happen the first time you would try to deploy it (for new users), and existing users should be reading release notes when updating to a new major. So I think it's doable as long as we are super explicit about it and it's impossible to miss the message.

Member

gaearon commented Jan 14, 2017

The problem is almost nobody looks at console in production. You can use any font there and people won't notice it.

However if we make it show a message by default in production as a breaking change (in 16 or 17) then it would be hard to miss. I mean, it would happen the first time you would try to deploy it (for new users), and existing users should be reading release notes when updating to a new major. So I think it's doable as long as we are super explicit about it and it's impossible to miss the message.

@addyosmani

This comment has been minimized.

Show comment
Hide comment
@addyosmani

addyosmani Jan 14, 2017

Contributor

The problem is almost nobody looks at console in production. You can use any font there and people won't notice it.

I can only comment on the Chrome team's experience with this, but I'd concur. Most folks will notice console messages during their iteration workflow but a far smaller % look at warnings on production sites and fewer act on them.

However if we make it show a message by default in production as a breaking change (in 16 or 17) then it would be hard to miss. I mean, it would happen the first time you would try to deploy it (for new users), and existing users should be reading release notes when updating to a new major. So I think it's doable as long as we are super explicit about it and it's impossible to miss the message.

Thanks for being open to a change like that @gaearon. What would it take to get agreement on trying for a message by default in a future release? 😄

Contributor

addyosmani commented Jan 14, 2017

The problem is almost nobody looks at console in production. You can use any font there and people won't notice it.

I can only comment on the Chrome team's experience with this, but I'd concur. Most folks will notice console messages during their iteration workflow but a far smaller % look at warnings on production sites and fewer act on them.

However if we make it show a message by default in production as a breaking change (in 16 or 17) then it would be hard to miss. I mean, it would happen the first time you would try to deploy it (for new users), and existing users should be reading release notes when updating to a new major. So I think it's doable as long as we are super explicit about it and it's impossible to miss the message.

Thanks for being open to a change like that @gaearon. What would it take to get agreement on trying for a message by default in a future release? 😄

@jakearchibald

This comment has been minimized.

Show comment
Hide comment
@jakearchibald

jakearchibald Jan 14, 2017

I agree that console warnings aren't the solution & a page-visible warning is much better.

The page-visible warning could:

  • Alert the dev that the site is in dev mode
  • Link to the docs on the benefits, and how to ship without it
  • A link to disable the message for… I dunno 2 hours?

Disabling the message is important, as it may be interfering/covering something on the page. Since this setting would be stored in localstorage, the warning will still appear on the live server because it's a different origin.

Yeah, it's pretty horrible if real users see this message on live sites, but it feels like the kind of problem devs will be encouraged to fix, whereas some seem to be happy to live with the performance issues of dev mode.

jakearchibald commented Jan 14, 2017

I agree that console warnings aren't the solution & a page-visible warning is much better.

The page-visible warning could:

  • Alert the dev that the site is in dev mode
  • Link to the docs on the benefits, and how to ship without it
  • A link to disable the message for… I dunno 2 hours?

Disabling the message is important, as it may be interfering/covering something on the page. Since this setting would be stored in localstorage, the warning will still appear on the live server because it's a different origin.

Yeah, it's pretty horrible if real users see this message on live sites, but it feels like the kind of problem devs will be encouraged to fix, whereas some seem to be happy to live with the performance issues of dev mode.

@rtorr

This comment has been minimized.

Show comment
Hide comment
@rtorr

rtorr Jan 14, 2017

If the first time I saw that warning (the insert into DOM one), was in production, I would be fairly upset. The warnings need to happen ahead of time.

rtorr commented Jan 14, 2017

If the first time I saw that warning (the insert into DOM one), was in production, I would be fairly upset. The warnings need to happen ahead of time.

@jakearchibald

This comment has been minimized.

Show comment
Hide comment
@jakearchibald

jakearchibald Jan 14, 2017

@rtorr my suggestion is that it happens whenever the site is in dev mode, so it should be seen ahead of time unless I'm missing something?

@rtorr my suggestion is that it happens whenever the site is in dev mode, so it should be seen ahead of time unless I'm missing something?

@rtorr

This comment has been minimized.

Show comment
Hide comment
@rtorr

rtorr Jan 14, 2017

@jakearchibald sorry for the confusion, my reply was not directed at yours. I just want to point out to the thread that if we were to use the 'insert into the dom' solution, we should be very careful and make sure users know before they push a thing (some how, I have no good idea here).

I just see some dev forgetting a setting or something and management freaks out. Is that possibility worth it for the consequences of having dev mode in production?

rtorr commented Jan 14, 2017

@jakearchibald sorry for the confusion, my reply was not directed at yours. I just want to point out to the thread that if we were to use the 'insert into the dom' solution, we should be very careful and make sure users know before they push a thing (some how, I have no good idea here).

I just see some dev forgetting a setting or something and management freaks out. Is that possibility worth it for the consequences of having dev mode in production?

@Pajn

This comment has been minimized.

Show comment
Hide comment
@Pajn

Pajn Jan 14, 2017

A DOM based warning that I have to constantly disable is not okay, it must be possible to disable it forever, and maybe it should never show at all for localhost.

A thing that just hit me if it would be possible to have some kind of a flag in the browser that you have to enable to activate the devtools (maybe a big overlay in them with "Are you a developer? [Yes/No]") that the page can detect and only show the warning for developers. Worded correctly it might help with self-XXS attacks as well.

Pajn commented Jan 14, 2017

A DOM based warning that I have to constantly disable is not okay, it must be possible to disable it forever, and maybe it should never show at all for localhost.

A thing that just hit me if it would be possible to have some kind of a flag in the browser that you have to enable to activate the devtools (maybe a big overlay in them with "Are you a developer? [Yes/No]") that the page can detect and only show the warning for developers. Worded correctly it might help with self-XXS attacks as well.

@jakearchibald

This comment has been minimized.

Show comment
Hide comment
@jakearchibald

jakearchibald Jan 14, 2017

A DOM based warning that I have to constantly disable is not okay, , it must be possible to disable it forever

Sites launching with dev-mode on is also not okay. Maybe the message only needs to be dismissed once per day? But the longer the period, the more likely it is that'll it'll end up on live. If it can be disabled forever, we're right back where we started.

A DOM based warning that I have to constantly disable is not okay, , it must be possible to disable it forever

Sites launching with dev-mode on is also not okay. Maybe the message only needs to be dismissed once per day? But the longer the period, the more likely it is that'll it'll end up on live. If it can be disabled forever, we're right back where we started.

@rtorr

This comment has been minimized.

Show comment
Hide comment
@rtorr

rtorr Jan 14, 2017

I also don't think an unintended dom node in production is OK.

I think that either way, we will always have an edge case. If this problem happens all of the time, then maybe the delivery of dev mode is wrong. Although not ideal in a perfect world, but if we find prod mode this important that we are willing to modify someone's application, maybe it should be default and dev mode should be opt in.

rtorr commented Jan 14, 2017

I also don't think an unintended dom node in production is OK.

I think that either way, we will always have an edge case. If this problem happens all of the time, then maybe the delivery of dev mode is wrong. Although not ideal in a perfect world, but if we find prod mode this important that we are willing to modify someone's application, maybe it should be default and dev mode should be opt in.

@jakearchibald

This comment has been minimized.

Show comment
Hide comment
@jakearchibald

jakearchibald Jan 14, 2017

@rtorr

I also don't think an unintended dom node in production is OK.

Why? (I'm not saying you're wrong, would just like to hear your reasons)

@rtorr

I also don't think an unintended dom node in production is OK.

Why? (I'm not saying you're wrong, would just like to hear your reasons)

@ropilz

This comment has been minimized.

Show comment
Hide comment
@ropilz

ropilz Jan 14, 2017

Maybe adding a setting to define prod Domain. If prod Domain is not set then we always get the warning about DEV mode (with a request to set Domain URL), if it is set then we only get warning when the URL matches with prod domain. We could even bind any service we want to notify devs

ropilz commented Jan 14, 2017

Maybe adding a setting to define prod Domain. If prod Domain is not set then we always get the warning about DEV mode (with a request to set Domain URL), if it is set then we only get warning when the URL matches with prod domain. We could even bind any service we want to notify devs

@addyosmani

This comment has been minimized.

Show comment
Hide comment
@addyosmani

addyosmani Jan 14, 2017

Contributor

I'm glad there's constructive discussion here. There are two solutions here that I can see solving the problem. Webpack could force specifying NODE_ENV which React could then use to more easily avoid folks shipping DEV to PROD, but that would be a breaking change to Webpack. I'm talking to Sean now about how feasible something like that could be for Webpack 3. Keeping the React + Webpack stack beginner and perf friendly is something I know both camps care about.

The second (DOM injection idea) is something React could do and as Jake mentioned, balance the UX by allowing the message to be shown once a day or be dismissed. It's a one line change to fix the issue then you just have to redeploy. I completely empathise with not wanting management to freak out.

If we're to get more React sites shipping the far faster experience FB worked on to prod something might have to give. If anyone has better ideas please suggest them.

Contributor

addyosmani commented Jan 14, 2017

I'm glad there's constructive discussion here. There are two solutions here that I can see solving the problem. Webpack could force specifying NODE_ENV which React could then use to more easily avoid folks shipping DEV to PROD, but that would be a breaking change to Webpack. I'm talking to Sean now about how feasible something like that could be for Webpack 3. Keeping the React + Webpack stack beginner and perf friendly is something I know both camps care about.

The second (DOM injection idea) is something React could do and as Jake mentioned, balance the UX by allowing the message to be shown once a day or be dismissed. It's a one line change to fix the issue then you just have to redeploy. I completely empathise with not wanting management to freak out.

If we're to get more React sites shipping the far faster experience FB worked on to prod something might have to give. If anyone has better ideas please suggest them.

@rtorr

This comment has been minimized.

Show comment
Hide comment
@rtorr

rtorr Jan 14, 2017

@jakearchibald

Why? (I'm not saying you're wrong, would just like to hear your reasons)

Back to my comment above, unless we are able to let developers know ahead of time (which seems to be the actual problem to solve), I find it kind of extreme to devalue someones product by displaying a warning to developers on their production page. In a lot of cases this could potentially hurt the product more than the performance of dev mode.

No matter what we do, someone is going to ship whatever is default into production, why not make production default? Why not improve dev mode to the point where it's not that big of a impact?

rtorr commented Jan 14, 2017

@jakearchibald

Why? (I'm not saying you're wrong, would just like to hear your reasons)

Back to my comment above, unless we are able to let developers know ahead of time (which seems to be the actual problem to solve), I find it kind of extreme to devalue someones product by displaying a warning to developers on their production page. In a lot of cases this could potentially hurt the product more than the performance of dev mode.

No matter what we do, someone is going to ship whatever is default into production, why not make production default? Why not improve dev mode to the point where it's not that big of a impact?

@rtorr

This comment has been minimized.

Show comment
Hide comment
@rtorr

rtorr Jan 14, 2017

@jakearchibald yeah, I see both sides have problems. I have faith of the people in this thread to come up with something good, even if it's what you are proposing. Y'all are great FYI. Maybe extreme is the answer.

rtorr commented Jan 14, 2017

@jakearchibald yeah, I see both sides have problems. I have faith of the people in this thread to come up with something good, even if it's what you are proposing. Y'all are great FYI. Maybe extreme is the answer.

@dan-gamble

This comment has been minimized.

Show comment
Hide comment
@dan-gamble

dan-gamble Jan 14, 2017

Could you not insert the DOM node warning if the user is running React dev tools so normal users don't experience it?

Could you not insert the DOM node warning if the user is running React dev tools so normal users don't experience it?

@Pajn

This comment has been minimized.

Show comment
Hide comment
@Pajn

Pajn Jan 14, 2017

@jakearchibald

Sites launching with dev-mode on is also not okay. Maybe the message only needs to be dismissed once per day? But the longer the period, the more likely it is that'll it'll end up on live. If it can be disabled forever, we're right back where we started.

When the site launched someone took a decision that it was "ready" so while it's bad it is not a catastrophe. However having (possibly, I don't know the exact numbers) hundreds of thousands of devs having to dismiss a site-destroying (A DOM warning must be treated as that as you have no idea of how it interacts with the rest of the site and if the site is usable with the warning visible) warning five or even just one time per day is a catastrophe. Most devs have a correctly set up build-chain (custom, create-react-app or other) and have no use at all for that warning, they need to be able to get rid of it.

@dan-gamble I beleave devs not using React Dev Tools is the most urgent target for this warning though.

Pajn commented Jan 14, 2017

@jakearchibald

Sites launching with dev-mode on is also not okay. Maybe the message only needs to be dismissed once per day? But the longer the period, the more likely it is that'll it'll end up on live. If it can be disabled forever, we're right back where we started.

When the site launched someone took a decision that it was "ready" so while it's bad it is not a catastrophe. However having (possibly, I don't know the exact numbers) hundreds of thousands of devs having to dismiss a site-destroying (A DOM warning must be treated as that as you have no idea of how it interacts with the rest of the site and if the site is usable with the warning visible) warning five or even just one time per day is a catastrophe. Most devs have a correctly set up build-chain (custom, create-react-app or other) and have no use at all for that warning, they need to be able to get rid of it.

@dan-gamble I beleave devs not using React Dev Tools is the most urgent target for this warning though.

@dan-gamble

This comment has been minimized.

Show comment
Hide comment
@dan-gamble

dan-gamble Jan 14, 2017

@Pajn Potentially, i don't think just because you download a Chrome extension it automatically makes you conscious of the prod / dev switch

@Pajn Potentially, i don't think just because you download a Chrome extension it automatically makes you conscious of the prod / dev switch

@Pajn

This comment has been minimized.

Show comment
Hide comment
@Pajn

Pajn Jan 14, 2017

@dan-gamble No, of course not. But there are people that develops a whole app without it which I think indicates that they don't use dev tools much and are therefore less likely to see the current warning for minified code.

Pajn commented Jan 14, 2017

@dan-gamble No, of course not. But there are people that develops a whole app without it which I think indicates that they don't use dev tools much and are therefore less likely to see the current warning for minified code.

@jlongster

This comment has been minimized.

Show comment
Hide comment
@jlongster

jlongster Jan 14, 2017

Contributor

I don't usually add to such a long discussion when I feel like the point has already been brought but this I have to agree with and want to emphasize this point: React touching the DOM and warning me I'm using a dev version would be a big mistake. A far as I know there is no precedent for that in any other framework.

Imagine all of the tutorials, all of the playgrounds, all of the little side projects that use dev mode to teach React. Every single little test site that I throw together to explore something fun in React, or try to isolate a test case. If React through up a warning on every single one of these sites that I had to manually disable I would be incredibly mad. It would feel like an overbearing parent and actively discourages you to use React because whenever you try to do something new it slaps you on the wrist.

Even doing it every 2 hours? No thank you. A constant nagging like this is certainly going to discourage users from developing in React and I honestly think it would push people away to adopt other frameworks. Maybe that's what the Chrome devs want?

Not to mention the fact that yes, this will slip into production somehow, and it's already hard enough to convince certain teams to adopt React and that's just more ammo for them against it.

The thing I love most about React is that that it doesn't do anything until you call ReactDOM.render(...) and when you do that, it only puts stuff into the DOM where you told it to. That's why it's such a good, isolated, functional library.

Do we also need to detect if people shipping an unminified bundle? What about if they aren't caching when they should? What about when they haven't configured nginx optimally? Or don't use shouldComponentUpdate when they should? Or are unnecessarily rendering everything when they don't need to?

There are several reasons for bad performance and to blame it solely on React's dev mode is wrong. When you deploy a site it's totally expected that you understand how to deploy an optimized site. I'm not saying there aren't things we can do, but the core reason for this is issue is benchmark authors not doing their due diligence and we shouldn't have to pay for that. We need to find another way to call that out.

Contributor

jlongster commented Jan 14, 2017

I don't usually add to such a long discussion when I feel like the point has already been brought but this I have to agree with and want to emphasize this point: React touching the DOM and warning me I'm using a dev version would be a big mistake. A far as I know there is no precedent for that in any other framework.

Imagine all of the tutorials, all of the playgrounds, all of the little side projects that use dev mode to teach React. Every single little test site that I throw together to explore something fun in React, or try to isolate a test case. If React through up a warning on every single one of these sites that I had to manually disable I would be incredibly mad. It would feel like an overbearing parent and actively discourages you to use React because whenever you try to do something new it slaps you on the wrist.

Even doing it every 2 hours? No thank you. A constant nagging like this is certainly going to discourage users from developing in React and I honestly think it would push people away to adopt other frameworks. Maybe that's what the Chrome devs want?

Not to mention the fact that yes, this will slip into production somehow, and it's already hard enough to convince certain teams to adopt React and that's just more ammo for them against it.

The thing I love most about React is that that it doesn't do anything until you call ReactDOM.render(...) and when you do that, it only puts stuff into the DOM where you told it to. That's why it's such a good, isolated, functional library.

Do we also need to detect if people shipping an unminified bundle? What about if they aren't caching when they should? What about when they haven't configured nginx optimally? Or don't use shouldComponentUpdate when they should? Or are unnecessarily rendering everything when they don't need to?

There are several reasons for bad performance and to blame it solely on React's dev mode is wrong. When you deploy a site it's totally expected that you understand how to deploy an optimized site. I'm not saying there aren't things we can do, but the core reason for this is issue is benchmark authors not doing their due diligence and we shouldn't have to pay for that. We need to find another way to call that out.

@jlongster

This comment has been minimized.

Show comment
Hide comment
@jlongster

jlongster Jan 14, 2017

Contributor

I meant to follow-up with what I think is the right solution: put these kinds of restrictions in the tools. Making sure that webpack or whatever tool you use to build your site for production is where we should force these checks and I'm down with any kind of restrictions we want to place in the build process.

Contributor

jlongster commented Jan 14, 2017

I meant to follow-up with what I think is the right solution: put these kinds of restrictions in the tools. Making sure that webpack or whatever tool you use to build your site for production is where we should force these checks and I'm down with any kind of restrictions we want to place in the build process.

@matthewp

This comment has been minimized.

Show comment
Hide comment
@matthewp

matthewp Jan 14, 2017

With regard to webpack forcing a NODE_ENV setting (maybe there's an issue for this in their repo already), wouldn't that make it harder to use libraries that don't rely on env settings?

Or is the idea that it would detect NODE_ENV use and force it only if the code uses it?

matthewp commented Jan 14, 2017

With regard to webpack forcing a NODE_ENV setting (maybe there's an issue for this in their repo already), wouldn't that make it harder to use libraries that don't rely on env settings?

Or is the idea that it would detect NODE_ENV use and force it only if the code uses it?

@jakearchibald

This comment has been minimized.

Show comment
Hide comment
@jakearchibald

jakearchibald Jan 14, 2017

Let's not get too hung up on the "2 hours" thing. It could be any period of time as long as it works.

Additionally, local storage events means it would only need to be dismissed once per origin. If you have multiple demos on one page, dismissing one would dismiss the others.

If the warning does slip into live, it's loud enough to warrant a quick fix - one that benefits users. If we're worried about how React looks in public, we really want to avoid unnecessary slow-down like this.

Sure, this wouldn't detect bad caching headers etc, this is about detecting "dev mode" only. Also "slippery slope" arguments aren't helpful.

I don't think moving the problem to build tools is useful, as you'll have the same problem if devs use a different build tool, or fail to put that into production mode. Dev mode frameworks already produce console warnings, and that clearly isn't working.

This isn't just about benchmarks, it's about real websites, running slow for real users, because a switch wasn't switched.

Let's not get too hung up on the "2 hours" thing. It could be any period of time as long as it works.

Additionally, local storage events means it would only need to be dismissed once per origin. If you have multiple demos on one page, dismissing one would dismiss the others.

If the warning does slip into live, it's loud enough to warrant a quick fix - one that benefits users. If we're worried about how React looks in public, we really want to avoid unnecessary slow-down like this.

Sure, this wouldn't detect bad caching headers etc, this is about detecting "dev mode" only. Also "slippery slope" arguments aren't helpful.

I don't think moving the problem to build tools is useful, as you'll have the same problem if devs use a different build tool, or fail to put that into production mode. Dev mode frameworks already produce console warnings, and that clearly isn't working.

This isn't just about benchmarks, it's about real websites, running slow for real users, because a switch wasn't switched.

@jlongster

This comment has been minimized.

Show comment
Hide comment
@jlongster

jlongster Jan 14, 2017

Contributor

@jakearchibald Thanks for the rational response to my emotionally-charged one. I do think that it is a valid point that there are many reasons a site might be slow with React. I'd like to see a way we can suggest performance improvements that's better than a crude check for dev mode and an in-browser warning. If we had tooling to analyze a React app and provide serious suggestions for performance, from everything like dev mode to too many rerenderings, that would be a lot more useful. A generic tool like that can be hooked into any pipeline, be it webpack, browserify, etc.

This is the main thing I wanted to say though: some days I will use React dev mode across 5-10 different places, like jsbin, tutorials, and even throwing together a small test site and opening it with the file:// protocol. A forced in-browser warning is hostile to this sort of flexible development which is what the web so excels at. I'd be seeing these warnings everywhere because I'm learning React across domains on the web.

Contributor

jlongster commented Jan 14, 2017

@jakearchibald Thanks for the rational response to my emotionally-charged one. I do think that it is a valid point that there are many reasons a site might be slow with React. I'd like to see a way we can suggest performance improvements that's better than a crude check for dev mode and an in-browser warning. If we had tooling to analyze a React app and provide serious suggestions for performance, from everything like dev mode to too many rerenderings, that would be a lot more useful. A generic tool like that can be hooked into any pipeline, be it webpack, browserify, etc.

This is the main thing I wanted to say though: some days I will use React dev mode across 5-10 different places, like jsbin, tutorials, and even throwing together a small test site and opening it with the file:// protocol. A forced in-browser warning is hostile to this sort of flexible development which is what the web so excels at. I'd be seeing these warnings everywhere because I'm learning React across domains on the web.

@TheLarkInn

This comment has been minimized.

Show comment
Hide comment
@TheLarkInn

TheLarkInn Jan 19, 2017

@taion Yes/No

-p only applies production "treatments" to your code, however we do not make any assumptions about and/nor have any knowledge of what NODE_ENV is set to. This is what brings upon the need for people to use DefinePlugin().

But I do think it is the closest "reasonable" area to guess or imply that the user is running their code in a production ENV. That would be the one area that we would want make sure the community and team is okay with.

TheLarkInn commented Jan 19, 2017

@taion Yes/No

-p only applies production "treatments" to your code, however we do not make any assumptions about and/nor have any knowledge of what NODE_ENV is set to. This is what brings upon the need for people to use DefinePlugin().

But I do think it is the closest "reasonable" area to guess or imply that the user is running their code in a production ENV. That would be the one area that we would want make sure the community and team is okay with.

@taion

This comment has been minimized.

Show comment
Hide comment
@TheLarkInn

This comment has been minimized.

Show comment
Hide comment
@TheLarkInn

TheLarkInn Jan 19, 2017

Ah sorry that's right I'm mistaken. It however it's not used frequently because people want more fine grained control over what they optimize. (Like @addyosmani mentioned)

TheLarkInn commented Jan 19, 2017

Ah sorry that's right I'm mistaken. It however it's not used frequently because people want more fine grained control over what they optimize. (Like @addyosmani mentioned)

@taion

This comment has been minimized.

Show comment
Hide comment
@taion

taion Jan 19, 2017

Is that really so common? When I was getting started with webpack, -p clearly seemed like the way to go. Like I referenced above, even libraries with plenty of reason to apply further tweaks still use -p.

taion commented Jan 19, 2017

Is that really so common? When I was getting started with webpack, -p clearly seemed like the way to go. Like I referenced above, even libraries with plenty of reason to apply further tweaks still use -p.

@addyosmani

This comment has been minimized.

Show comment
Hide comment
@addyosmani

addyosmani Jan 21, 2017

Contributor

We could create a new resolver pattern that will check React's (or any fw that needs it) package.json for something like below:

"webpack": {
"plugin": "ReactEnviornmentPlugin"
}
that would automatically apply to a users compiler configuration without them needing to know or care.

@TheLarkInn if I'm reading this correctly, to trigger the resolver pattern an app's package.json would need to manually specify the ReactEnvironmentPlugin, right? Or am I misunderstanding the proposal? :)

I could imagine some resolution magic in Webpack that detects a project is using React and does the right environment switching for them, but that sounds like a tight coupling of framework-specific optimisation to a bundler and might not be as desirable.

Contributor

addyosmani commented Jan 21, 2017

We could create a new resolver pattern that will check React's (or any fw that needs it) package.json for something like below:

"webpack": {
"plugin": "ReactEnviornmentPlugin"
}
that would automatically apply to a users compiler configuration without them needing to know or care.

@TheLarkInn if I'm reading this correctly, to trigger the resolver pattern an app's package.json would need to manually specify the ReactEnvironmentPlugin, right? Or am I misunderstanding the proposal? :)

I could imagine some resolution magic in Webpack that detects a project is using React and does the right environment switching for them, but that sounds like a tight coupling of framework-specific optimisation to a bundler and might not be as desirable.

@TheLarkInn

This comment has been minimized.

Show comment
Hide comment
@TheLarkInn

TheLarkInn Jan 21, 2017

Less that it would detect react, and more that it would detect a js modules description fields include a webpack field with a plugin. But you are right, it is very tightly coupled and not really my favorite idea either.

Less that it would detect react, and more that it would detect a js modules description fields include a webpack field with a plugin. But you are right, it is very tightly coupled and not really my favorite idea either.

@taion

This comment has been minimized.

Show comment
Hide comment
@taion

taion Jan 21, 2017

I don't think this really gives you any guarantees unless webpack has a concept of "production" mode to figure out how to configure React – and this seems redundant given that, as discussed above, -p already does the right thing, and is what users will generally reach for when making a minified production build with webpack anyway.

taion commented Jan 21, 2017

I don't think this really gives you any guarantees unless webpack has a concept of "production" mode to figure out how to configure React – and this seems redundant given that, as discussed above, -p already does the right thing, and is what users will generally reach for when making a minified production build with webpack anyway.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jan 24, 2017

Member

We’ve talked about this a bit more and I think there is a reasonable solution.

We’ve long considered enabling a “warning box” for React warnings in development. You can see a demo here (PR: #7360). It only shows up when you have warnings but they are very common during development (and should always be fixed), so presumably anyone who spent more than five minutes developing an app will see the dialog and be aware that it exists.

After this change, it will be harder to be unaware of the development mode. You’ll likely search for “how to remove the warning dialog” and learn about building for production. If you don’t, you’re likely to get some warnings deployed at some point, and your users will see them. I think that in this case people won’t blame React per se because we don’t just show the box to be obnoxious—we just do what the development mode is supposed to.

(By the way, we’ve been using a similar warning box in development at Facebook for a long time so it corresponds to how we intend React to be used.)

Member

gaearon commented Jan 24, 2017

We’ve talked about this a bit more and I think there is a reasonable solution.

We’ve long considered enabling a “warning box” for React warnings in development. You can see a demo here (PR: #7360). It only shows up when you have warnings but they are very common during development (and should always be fixed), so presumably anyone who spent more than five minutes developing an app will see the dialog and be aware that it exists.

After this change, it will be harder to be unaware of the development mode. You’ll likely search for “how to remove the warning dialog” and learn about building for production. If you don’t, you’re likely to get some warnings deployed at some point, and your users will see them. I think that in this case people won’t blame React per se because we don’t just show the box to be obnoxious—we just do what the development mode is supposed to.

(By the way, we’ve been using a similar warning box in development at Facebook for a long time so it corresponds to how we intend React to be used.)

@surma

This comment has been minimized.

Show comment
Hide comment
@surma

surma Jan 24, 2017

I am really happy to see that proposal, @gaearon! It’s everything I dreamed it to be ;)

Just as an side: Maybe it’s worth considering having a link directly in the box to not require the developers to google for how to get rid of it.

surma commented Jan 24, 2017

I am really happy to see that proposal, @gaearon! It’s everything I dreamed it to be ;)

Just as an side: Maybe it’s worth considering having a link directly in the box to not require the developers to google for how to get rid of it.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jan 24, 2017

Member

Yea, good point. We'll add something.

Member

gaearon commented Jan 24, 2017

Yea, good point. We'll add something.

@KrisSiegel

This comment has been minimized.

Show comment
Hide comment
@KrisSiegel

KrisSiegel Jan 24, 2017

@gaearon I find that solution highly obnoxious; I would never want warnings to invade the DOM even during development. That serves zero purpose for the common developer and would likely be disabled in its entirely by most. The developer tools display warnings for a reason, they don't need to be invented.

I also find it troubling that DOM solutions keep cropping up with zero rebuttals to any of my prior arguments against it. If my points are to be ignored then fine, this isn't my repository so that's fair enough, but it's disheartening to see the same argument come up, people post against it, people seem for the points as there is never an argument against them, then they just come right back up. Rinse, repeat.

@gaearon I find that solution highly obnoxious; I would never want warnings to invade the DOM even during development. That serves zero purpose for the common developer and would likely be disabled in its entirely by most. The developer tools display warnings for a reason, they don't need to be invented.

I also find it troubling that DOM solutions keep cropping up with zero rebuttals to any of my prior arguments against it. If my points are to be ignored then fine, this isn't my repository so that's fair enough, but it's disheartening to see the same argument come up, people post against it, people seem for the points as there is never an argument against them, then they just come right back up. Rinse, repeat.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jan 24, 2017

Member

While I agree about this being true about most warnings, React warnings specifically point out the bugs in your code. They are not just suggestions.

The dialog is easy to dismiss and the individual warnings are easy to snooze (in case you don't care about them for a while) but they need to be fixed.

For comparison, this is how the dialog looks in the Facebook codebase:

screen shot 2017-01-24 at 17 55 47

Thousands of engineers don’t have a problem with it and are more productive thanks to it, so we think it’s a reasonable default. To be clear, the open source version will be less shouting:

screen shot 2017-01-24 at 17 57 14

If you have suggestions about style tweaks please feel free to comment on #7360.

Member

gaearon commented Jan 24, 2017

While I agree about this being true about most warnings, React warnings specifically point out the bugs in your code. They are not just suggestions.

The dialog is easy to dismiss and the individual warnings are easy to snooze (in case you don't care about them for a while) but they need to be fixed.

For comparison, this is how the dialog looks in the Facebook codebase:

screen shot 2017-01-24 at 17 55 47

Thousands of engineers don’t have a problem with it and are more productive thanks to it, so we think it’s a reasonable default. To be clear, the open source version will be less shouting:

screen shot 2017-01-24 at 17 57 14

If you have suggestions about style tweaks please feel free to comment on #7360.

@bvaughn

This comment has been minimized.

Show comment
Hide comment
@bvaughn

bvaughn Jan 24, 2017

Contributor

Adding onto what Dan said, I'm building on top of the #7360 to hook into our recently-added error logger flow. I'm currently trying out a couple of styles of toast notifications that are a little less intrusive. I'll post some screenshots and/or a Plnkr soon for feedback.

Contributor

bvaughn commented Jan 24, 2017

Adding onto what Dan said, I'm building on top of the #7360 to hook into our recently-added error logger flow. I'm currently trying out a couple of styles of toast notifications that are a little less intrusive. I'll post some screenshots and/or a Plnkr soon for feedback.

@taion

This comment has been minimized.

Show comment
Hide comment
@taion

taion Jan 24, 2017

Would these warnings include the "minified dev code" warning? That'd solve a lot of things quite neatly if so.

taion commented Jan 24, 2017

Would these warnings include the "minified dev code" warning? That'd solve a lot of things quite neatly if so.

@bvaughn

This comment has been minimized.

Show comment
Hide comment
@bvaughn

bvaughn Jan 24, 2017

Contributor

I don't see why it couldn't also be used for that purpose.

Contributor

bvaughn commented Jan 24, 2017

I don't see why it couldn't also be used for that purpose.

@KrisSiegel

This comment has been minimized.

Show comment
Hide comment
@KrisSiegel

KrisSiegel Jan 24, 2017

@gaearon

While I agree about this being true about most warnings, React warnings specifically point out the bugs in your code. They are not just suggestions.

Those should be errors or exceptions IMO. Why are they not? Exceptions force things to be corrected but a dismissible warning does not.

Thousands of engineers don’t have a problem with it and are more productive thanks to it, so we think it’s a reasonable default.

I mentioned this in a prior point but I would guess those engineers are likely better than a good 90% of people who will use the open source version. I find it the opposite of a reasonable default. There is a reason development tools have warnings; reinventing them doesn't make sense to me. It'll get disabled and never seen again.

This just looks like React is trying to do too much IMO.


Anyway, I'm just repeating my arguments that I've already said twice. If you're going to go ahead then feel free. In my opinion it's not a good business to get into. I'll just leave this short anecdote about a time when I wanted to punch some toast...

When I did government contracting we had a common library all front ends had to use. It would take errors in the console and display them as a toast pop up. No only did it get deployed to production multiple times by multiple teams but many developers saw it once then asked how to permanently disabled it. I see this as more of the same.

@gaearon

While I agree about this being true about most warnings, React warnings specifically point out the bugs in your code. They are not just suggestions.

Those should be errors or exceptions IMO. Why are they not? Exceptions force things to be corrected but a dismissible warning does not.

Thousands of engineers don’t have a problem with it and are more productive thanks to it, so we think it’s a reasonable default.

I mentioned this in a prior point but I would guess those engineers are likely better than a good 90% of people who will use the open source version. I find it the opposite of a reasonable default. There is a reason development tools have warnings; reinventing them doesn't make sense to me. It'll get disabled and never seen again.

This just looks like React is trying to do too much IMO.


Anyway, I'm just repeating my arguments that I've already said twice. If you're going to go ahead then feel free. In my opinion it's not a good business to get into. I'll just leave this short anecdote about a time when I wanted to punch some toast...

When I did government contracting we had a common library all front ends had to use. It would take errors in the console and display them as a toast pop up. No only did it get deployed to production multiple times by multiple teams but many developers saw it once then asked how to permanently disabled it. I see this as more of the same.

@addyosmani

This comment has been minimized.

Show comment
Hide comment
@addyosmani

addyosmani Jan 24, 2017

Contributor

We’ve long considered enabling a “warning box” for React warnings in development. You can see a demo here (PR: #7360). It only shows up when you have warnings but they are very common during development (and should always be fixed), so presumably anyone who spent more than five minutes developing an app will see the dialog and be aware that it exists.

I really like #7360, @gaearon. It's heartening to hear support for highlighting the need to switch to PROD for deployments in the new warning box. It's nice and visual.

Adding onto what Dan said, I'm building on top of the #7360 to hook into our recently-added error logger flow. I'm currently trying out a couple of styles of toast notifications that are a little less intrusive. I'll post some screenshots and/or a Plnkr soon for feedback.

@bvaughn Looking forward to seeing more of your iterations :)

For folks who feel the warning box approach may be too intrusive, other libraries (e.g VueJS) already display DOM overlays during your dev/iteration workflow to encourage bug-fixing or slow-paths:

screen shot 2017-01-24 at 10 57 11 am

My own experience has been that while it's a minor inconvenience, these messages are more obvious than what you might see in the console. I feel Dan's right that it'll at least place more emphasis on dev mode not being something you should deploy to prod and will hopefully lead to more sites shipping "faster mode" to their end users.

After this change, it will be harder to be unaware of the development mode. You’ll likely search for “how to remove the warning dialog” and learn about building for production. If you don’t, you’re likely to get some warnings deployed at some point, and your users will see them. I think that in this case people won’t blame React per se because we don’t just show the box to be obnoxious—we just do what the development mode is supposed to.

Contributor

addyosmani commented Jan 24, 2017

We’ve long considered enabling a “warning box” for React warnings in development. You can see a demo here (PR: #7360). It only shows up when you have warnings but they are very common during development (and should always be fixed), so presumably anyone who spent more than five minutes developing an app will see the dialog and be aware that it exists.

I really like #7360, @gaearon. It's heartening to hear support for highlighting the need to switch to PROD for deployments in the new warning box. It's nice and visual.

Adding onto what Dan said, I'm building on top of the #7360 to hook into our recently-added error logger flow. I'm currently trying out a couple of styles of toast notifications that are a little less intrusive. I'll post some screenshots and/or a Plnkr soon for feedback.

@bvaughn Looking forward to seeing more of your iterations :)

For folks who feel the warning box approach may be too intrusive, other libraries (e.g VueJS) already display DOM overlays during your dev/iteration workflow to encourage bug-fixing or slow-paths:

screen shot 2017-01-24 at 10 57 11 am

My own experience has been that while it's a minor inconvenience, these messages are more obvious than what you might see in the console. I feel Dan's right that it'll at least place more emphasis on dev mode not being something you should deploy to prod and will hopefully lead to more sites shipping "faster mode" to their end users.

After this change, it will be harder to be unaware of the development mode. You’ll likely search for “how to remove the warning dialog” and learn about building for production. If you don’t, you’re likely to get some warnings deployed at some point, and your users will see them. I think that in this case people won’t blame React per se because we don’t just show the box to be obnoxious—we just do what the development mode is supposed to.

@Pajn

This comment has been minimized.

Show comment
Hide comment
@Pajn

Pajn Jan 24, 2017

Those should be errors or exceptions IMO. Why are they not? Exceptions force things to be corrected but a dismissible warning does not.

While they should be fixed, I might have more urgent matters at hand. For example do I often prototype/mock up UIs and while doing that I write quick and usually sub-par code which React can warn about. While I want to fix those warnings, I don't really care until I know that I wont throw away all the code in the next hour at least. Forcing people to fix them instantly will drastically slow down experimental development.

Pajn commented Jan 24, 2017

Those should be errors or exceptions IMO. Why are they not? Exceptions force things to be corrected but a dismissible warning does not.

While they should be fixed, I might have more urgent matters at hand. For example do I often prototype/mock up UIs and while doing that I write quick and usually sub-par code which React can warn about. While I want to fix those warnings, I don't really care until I know that I wont throw away all the code in the next hour at least. Forcing people to fix them instantly will drastically slow down experimental development.

@glenjamin

This comment has been minimized.

Show comment
Hide comment
@glenjamin

glenjamin Jan 24, 2017

Contributor

For folks who feel the warning box approach may be too intrusive, other libraries (e.g VueJS) already display DOM overlays during your dev/iteration workflow to encourage bug-fixing or slow-paths:

screen shot 2017-01-24 at 10 57 11 am

Are you sure that's from Vue itself? It looks a lot like webpack build errors displayed with the error overlay from webpack-hot-middleware. If this is the case, it's subtly different because it's dev-time build tooling adding the overlay rather than a general-purpose frontend framework.

In general I'm in favour of the warning overlay, but I think it should contain explanatory text on it that says what it is, why it's there, and that it can & should be disabled as part of turning off dev mode. Behind an expando if that's a bit long - but it seems like as good a place as any to get the message across.

Contributor

glenjamin commented Jan 24, 2017

For folks who feel the warning box approach may be too intrusive, other libraries (e.g VueJS) already display DOM overlays during your dev/iteration workflow to encourage bug-fixing or slow-paths:

screen shot 2017-01-24 at 10 57 11 am

Are you sure that's from Vue itself? It looks a lot like webpack build errors displayed with the error overlay from webpack-hot-middleware. If this is the case, it's subtly different because it's dev-time build tooling adding the overlay rather than a general-purpose frontend framework.

In general I'm in favour of the warning overlay, but I think it should contain explanatory text on it that says what it is, why it's there, and that it can & should be disabled as part of turning off dev mode. Behind an expando if that's a bit long - but it seems like as good a place as any to get the message across.

@jquense

This comment has been minimized.

Show comment
Hide comment
@jquense

jquense Jan 28, 2017

Collaborator

I dread updates like 15.2.0 with an overlay. minor bump and suddenly you have literally 100s of warnings about props being passed to DOM nodes. errors maybe, but I don't think depreciation warnings belong in such an intrusive space

Collaborator

jquense commented Jan 28, 2017

I dread updates like 15.2.0 with an overlay. minor bump and suddenly you have literally 100s of warnings about props being passed to DOM nodes. errors maybe, but I don't think depreciation warnings belong in such an intrusive space

@bvaughn

This comment has been minimized.

Show comment
Hide comment
@bvaughn

bvaughn Jan 28, 2017

Contributor

errors maybe, but I don't think depreciation warnings belong in such an intrusive space

I don't know if this was very clearly communicated prior, but the idea regarding yellow-box warnings (#7360) was not to show all warnings (deprecation or other). Rather, certain warnings that the team deemed to be particularly important would be highlighted this way. The rest would presumably remain in the console.

At least that's my take-away from the conversation Tom and I had about this feature a week or two ago.

Contributor

bvaughn commented Jan 28, 2017

errors maybe, but I don't think depreciation warnings belong in such an intrusive space

I don't know if this was very clearly communicated prior, but the idea regarding yellow-box warnings (#7360) was not to show all warnings (deprecation or other). Rather, certain warnings that the team deemed to be particularly important would be highlighted this way. The rest would presumably remain in the console.

At least that's my take-away from the conversation Tom and I had about this feature a week or two ago.

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Jan 28, 2017

Member

The prop warning in 15.2 was also a mistake IMO and not indicative of our normal M.O. We'd like to have a way to control warning levels by minor versions to avoid that.

Member

sebmarkbage commented Jan 28, 2017

The prop warning in 15.2 was also a mistake IMO and not indicative of our normal M.O. We'd like to have a way to control warning levels by minor versions to avoid that.

@markboyall

This comment has been minimized.

Show comment
Hide comment
@markboyall

markboyall Feb 3, 2017

The main reason our team does not use the production build is because we can't run JS unit tests, since the test utils are not included.

The main reason our team does not use the production build is because we can't run JS unit tests, since the test utils are not included.

@addyosmani

This comment has been minimized.

Show comment
Hide comment
@addyosmani

addyosmani Mar 8, 2017

Contributor

First, another round of thanks to the React team (@sebmarkbage, @gaearon, @tomocchino and others) for discussing this issue with us and being so open to talking to us about performance & mobile at BlinkOn and other syncs this quarter.

Status check

Per @aweary in #7360, the Yellow Box solution to this particular problem has been put on hold until React’s high-prio V16 work gets completed but should still be happening. facebook/fbjs#165 needs to land and be implemented in Fiber. A good public API for exposing hooks also needs to be crafted. Will be keeping my fingers 🤞

This problem appears to still be prevalent

Quite a few of the production apps that have come across my desk are still shipping DEV mode to production. We can see the When deploying React apps to production debug string in their builds here:

https://cdnjs.cloudflare.com/ajax/libs/react/15.3.1/react.js (tombraider.com)
https://shared.reliclink.com/dlls/vendor-f3e016f6037eb107ffc0.live-shared.min.js (dawnofwar.com)
https://d1xx3pvec9nqb7.cloudfront.net/media/js/thread.af65c1a02d15.js (thread.com)
http://www.sothebys.com/etc/designs/redesigns/sothebys/redesignlibs.min.js (sothebys.com)

I'm still of the view that a pre-Yellow Box move to logging the DEV mode warning to console for the above might have some impact. Sebastian's suggestion of throwing a console error so crash reporting might pick these up was also something I felt was worth consideration.

What else can we do to move the needle here?

Better advocacy? I'm happy to commit to continuing to advocate for folks not shipping DEV mode to production, but do want to see if we can land the official solution post V16 :)

In the short-term, it looks like create-react-appis in a good place to help new projects avoid this problem.

Improvements to installation docs

For everyone else, would there be support for https://facebook.github.io/react/docs/installation.html including a clear, visible callout under the Installing React heading reminding folks to be mindful of DEV mode in production?

As a user, I don't feel there's great incentive for me to read https://facebook.github.io/react/docs/installation.html#development-and-production-versions on first glance otherwise.

Thoughts?

Contributor

addyosmani commented Mar 8, 2017

First, another round of thanks to the React team (@sebmarkbage, @gaearon, @tomocchino and others) for discussing this issue with us and being so open to talking to us about performance & mobile at BlinkOn and other syncs this quarter.

Status check

Per @aweary in #7360, the Yellow Box solution to this particular problem has been put on hold until React’s high-prio V16 work gets completed but should still be happening. facebook/fbjs#165 needs to land and be implemented in Fiber. A good public API for exposing hooks also needs to be crafted. Will be keeping my fingers 🤞

This problem appears to still be prevalent

Quite a few of the production apps that have come across my desk are still shipping DEV mode to production. We can see the When deploying React apps to production debug string in their builds here:

https://cdnjs.cloudflare.com/ajax/libs/react/15.3.1/react.js (tombraider.com)
https://shared.reliclink.com/dlls/vendor-f3e016f6037eb107ffc0.live-shared.min.js (dawnofwar.com)
https://d1xx3pvec9nqb7.cloudfront.net/media/js/thread.af65c1a02d15.js (thread.com)
http://www.sothebys.com/etc/designs/redesigns/sothebys/redesignlibs.min.js (sothebys.com)

I'm still of the view that a pre-Yellow Box move to logging the DEV mode warning to console for the above might have some impact. Sebastian's suggestion of throwing a console error so crash reporting might pick these up was also something I felt was worth consideration.

What else can we do to move the needle here?

Better advocacy? I'm happy to commit to continuing to advocate for folks not shipping DEV mode to production, but do want to see if we can land the official solution post V16 :)

In the short-term, it looks like create-react-appis in a good place to help new projects avoid this problem.

Improvements to installation docs

For everyone else, would there be support for https://facebook.github.io/react/docs/installation.html including a clear, visible callout under the Installing React heading reminding folks to be mindful of DEV mode in production?

As a user, I don't feel there's great incentive for me to read https://facebook.github.io/react/docs/installation.html#development-and-production-versions on first glance otherwise.

Thoughts?

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Mar 8, 2017

Member

The main reason our team does not use the production build is because we can't run JS unit tests, since the test utils are not included.

I’m confused about this. Are you running tests on production website? If not, what prevents you from using production build on the production website, and development build in development?

Member

gaearon commented Mar 8, 2017

The main reason our team does not use the production build is because we can't run JS unit tests, since the test utils are not included.

I’m confused about this. Are you running tests on production website? If not, what prevents you from using production build on the production website, and development build in development?

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Mar 8, 2017

Member

For everyone else, would there be support for https://facebook.github.io/react/docs/installation.html including a clear, visible callout under the Installing React heading reminding folks to be mindful of DEV mode in production?

Sure. Want to send a PR?

Member

gaearon commented Mar 8, 2017

For everyone else, would there be support for https://facebook.github.io/react/docs/installation.html including a clear, visible callout under the Installing React heading reminding folks to be mindful of DEV mode in production?

Sure. Want to send a PR?

@addyosmani

This comment has been minimized.

Show comment
Hide comment
@addyosmani

addyosmani Mar 9, 2017

Contributor

Sure. Want to send a PR?

More than happy to.

Contributor

addyosmani commented Mar 9, 2017

Sure. Want to send a PR?

More than happy to.

addyosmani added a commit to addyosmani/react that referenced this issue Mar 11, 2017

@jide

This comment has been minimized.

Show comment
Hide comment
@jide

jide Mar 13, 2017

Maybe a word about benchmarks would be nice too to help educating those who compare perfs with react in dev mode ?

jide commented Mar 13, 2017

Maybe a word about benchmarks would be nice too to help educating those who compare perfs with react in dev mode ?

@bvaughn

This comment has been minimized.

Show comment
Hide comment
@bvaughn

bvaughn Apr 29, 2017

Contributor

Makes me think the react devtools extension could be leveraged to display a notification or something obvious when opening a page using react in dev mode maybe ?

I like this idea! I put together a set of proposed icons for the devtools (see facebook/react-devtools#652).

We need to decide how to detect dev vs prod React in a way that's backwards and future safe, but I've added it to the Monday meeting agenda.

Contributor

bvaughn commented Apr 29, 2017

Makes me think the react devtools extension could be leveraged to display a notification or something obvious when opening a page using react in dev mode maybe ?

I like this idea! I put together a set of proposed icons for the devtools (see facebook/react-devtools#652).

We need to decide how to detect dev vs prod React in a way that's backwards and future safe, but I've added it to the Monday meeting agenda.

gaearon added a commit that referenced this issue May 3, 2017

Add DEV mode note to installation doc (#8784) (#9157)
* Add DEV mode note to installation doc (#8784)

* Address feedback from Dan on wording

gaearon added a commit that referenced this issue May 3, 2017

Add DEV mode note to installation doc (#8784) (#9157)
* Add DEV mode note to installation doc (#8784)

* Address feedback from Dan on wording

(cherry picked from commit f86256e)

flarnie added a commit that referenced this issue Jun 12, 2017

Add DEV mode note to installation doc (#8784) (#9157)
* Add DEV mode note to installation doc (#8784)

* Address feedback from Dan on wording

(cherry picked from commit f86256e)

@rwjblue rwjblue referenced this issue in ember-cli/rfcs Jul 24, 2017

Open

Environment Selection #109

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jul 26, 2017

Member

We’ve done some reasonable steps to address this problem:

  • React DevTools (with ~700K users) now displays a distinctive red icon for development builds. This helps people learn about the difference between versions early. It also creates some peer pressure, as developers notice this on sites they visit, and report to the people working on them. We’ve seen a few major sites fix the issue within days of rolling this out.

  • The notice in React DevTools links to our website where we published instructions to create the production build for all major bundlers. We also made it more prominent on the installation page.

  • Create React App continued to gain popularity. It teaches this distinction early with separate commands. It also displays a permanent notice about development mode in the terminal.

  • React 16 Beta 1 (and further releases) ship with react.development.js and react.production.min.js as filenames to make it clear that non-minified build should not be used in production.

I think in the future we might explore more ways to solve this problem, but for now I feel like we can move ahead without more drastic measures, and see if it helps. Thanks to everyone for the discussion.

Member

gaearon commented Jul 26, 2017

We’ve done some reasonable steps to address this problem:

  • React DevTools (with ~700K users) now displays a distinctive red icon for development builds. This helps people learn about the difference between versions early. It also creates some peer pressure, as developers notice this on sites they visit, and report to the people working on them. We’ve seen a few major sites fix the issue within days of rolling this out.

  • The notice in React DevTools links to our website where we published instructions to create the production build for all major bundlers. We also made it more prominent on the installation page.

  • Create React App continued to gain popularity. It teaches this distinction early with separate commands. It also displays a permanent notice about development mode in the terminal.

  • React 16 Beta 1 (and further releases) ship with react.development.js and react.production.min.js as filenames to make it clear that non-minified build should not be used in production.

I think in the future we might explore more ways to solve this problem, but for now I feel like we can move ahead without more drastic measures, and see if it helps. Thanks to everyone for the discussion.

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