facebook / react

The library for web and native user interfaces.
https://react.dev
MIT License
228.73k stars 46.82k forks source link

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

Closed addyosmani closed 7 years ago

addyosmani commented 7 years ago

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 commented 7 years ago

For reference: https://github.com/facebook/react/pull/8782

sebmarkbage commented 7 years ago

For context we already warn if we detect that you've minified a DEV version of React: https://github.com/facebook/react/blob/8791325db03725ef4801fc58b35a3bb4486a8904/src/renderers/dom/ReactDOM.js#L91-L98

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

sebmarkbage commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 7 years ago

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

aweary commented 7 years ago

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 commented 7 years ago

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 7 years ago
capture d ecran 2017-01-14 a 03 05 49
gaearon commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

The page-visible warning could:

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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

@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?

jakearchibald commented 7 years ago

@rtorr https://twitter.com/sebmarkbage/status/820054247496503296

rtorr commented 7 years ago

@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 commented 7 years ago

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

Pajn commented 7 years ago

@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 commented 7 years ago

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

Pajn commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

@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.

aweary commented 7 years ago

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.

Even allowing the possibility of a developer-specific warning being displayed to end users seems unacceptable to me. A slow site is one thing, but a message like this could undermine user trust, especially for security-focused sites (would you be happy if your banking site was displaying cryptic errors all of a sudden?) Would Google be OK with all of their users suddenly getting this warning, even if only for a moment?

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.

You can't rely on localStorage to dedupe this. There's no guarantee that localStorage (or any other local persistent data) will not be cleared at any interval.

edit: Also, by only displaying the error once every {INTERVAL} you've now made it far more difficult to reproduce and debug as it's not deterministically reproducible.

jide commented 7 years ago

There are 2 cases that need to be solved:

Arguments against touching the DOM are convincing.

If there is a big, flashy, obvious console warning, chances are that before shipping to production people will use the dev version and see this really obvious console message. Or maybe some code has already been shipped to production, but for another project they will use the next react version with this impossible to miss console message. Maybe they will remember that site they shipped to production and check if DEV is enabled there.

The console message would be educationnal, like, anyone who does React development knows that there's something really important about DEV, and they see it everytime they use React for development.

aweary commented 7 years ago

I was hesitant about https://github.com/facebook/react/pull/8782 because people generally don't like warnings they can't get rid of (see https://github.com/facebook/react/issues/3877), but considering the alternatives it might be an acceptable solution.

sebmarkbage commented 7 years ago

Curious. Would localStorage usage invoke the EU cookie law on a site that isn't otherwise covered by it?

If informative warnings during development is a good idea, then why aren't other libraries doing it? Well, one reason is for problems like this. If others wanted to do this should they also pop up similar UIs? Do you have to close all of them?

It seems to me that it would be ideal to have something more central to handle this.

Perhaps Chrome could have a development mode? Libraries could tell the host that they're in development mode and then Chrome could add a badge or pop up to indicate that.

jide commented 7 years ago

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 ?

jide commented 7 years ago

capture d ecran 2017-01-14 a 17 13 43

jakearchibald commented 7 years ago

@sebmarkbage

If informative warnings during development is a good idea, then why aren't other libraries doing it?

I guess someone needs to be the first. Angular has a similar problem, with stuff like http://code.gov launching in dev mode. If React starts catching this stuff where other frameworks don't, I'll be pushing for them to make similar changes.

aweary commented 7 years ago

I'll be pushing for them to make similar changes.

@jakearchibald are you suggesting that each framework should provide their own warning? I don't think setting the standard for frameworks/libraries providing their own development warnings in production is a great idea. Shouldn't we be trying to standardize on the platform? As mentioned by @sebmarkbage

Perhaps Chrome could have a development mode? Libraries could tell the host that they're in development mode and then Chrome could add a badge or pop up to indicate that.

gaearon commented 7 years ago

I think this is a great idea. Precedent: Safari has a separate mode that you must enable to access DevTools. If Chrome did the same then it could also safely add an indicator for DEV mode and an API to trigger it. This indicator would only be visible to developers so it wouldn't disrupt user experience.

jide commented 7 years ago

Isn't waiting for browser vendors to implement such a thing going to take time ?