Add DEV mode warning

[rauchg]

Just a quick patch to get the discussion started.

The warning reads:

React loaded in DEV mode. See https://fb.me/react-dev-mode for more details

• I'm not sure this is the best place to add this. Other warnings are present in the file, and it's inside isomorphic, so I made the decision based on that
• I decided not to use the built-in warning utility because I don't need any conditions. Thoughts?
  • Are we supporting environments where console.log should be checked for?
• Jest will output these warnings during tests. I've seen in other parts of the codebase there are checks for it that look like this. I decided not to include it because it's pretty verbose, but please let me know if I should change it

  if (
    typeof process !== 'undefined' &&
    process.env &&
    process.env.NODE_ENV === 'test'
  ) {
    // …
  }

• My suggestion is to create a page / redirection from https://fb.me/react-dev-mode to a place where some notes are collected around:
  • How to get rid of the warning
  • What the differences are between dev mode and prod mode
  • Tips for benchmark authors and perhaps an example of what's considered a useful benchmark
    • Perhaps useful links to articles like this one

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

If you are contributing on behalf of someone else (eg your employer): the individual CLA is not sufficient - use https://developers.facebook.com/opensource/cla?type=company instead. Contact cla@fb.com if you have any questions.

[rauchg]

Tangential concern: Jest does output the warnings, which means Jest runs React in DEV mode. Are there precautions in place to also run the tests in prod mode? Otherwise we might risk relying too much on developer discipline not to introduce semantic changes during development.

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[aweary]

Thanks for the PR @rauchg. I'm not sure I see the utility of adding an unconditional warning when using React in development. There's nothing inherently wrong with using React in dev mode. We already have a warning when using a minified copy of the development build. Is there another, specific situation you can think of where the current warning is insufficient and this new warning helps?

I suspect this would likely just annoy users who are well aware they are using React in dev mode (as they are developing their application).

[sebmarkbage]

I think what we need is a way to opt out of this once you've set up your environment. For example, should Create React App need this warning even though it has a verified environment set up? However, what if you use CRA's development mode to run benchmarks?

[aweary]

We should use warning to remain consistent with the rest of the codebase. I think there's other reasons we use warning specifically, not sure offhand what they are. Since this is unconditional you would just do:

warning(false, "message")

But we'd likely want to dedupe this error message if accepted. See how the React.__spread warning handles it below.

[rauchg]

@aweary my understanding is that deduping is happening there because it's a function that can be called many times. Isn't this file meant to be evaluated once?

[aweary]

@rauchg ah, you're right. good catch, I missed that it was in a function.

[spikebrehm]

Another distinction is that warning uses console.error(), rather than console.log():

(cf. https://github.com/BerkeleyTrue/warning/blob/master/warning.js#L38)

However IMO console.warn() would be the most appropriate in this case.

[rauchg]

I also couldn't decide if it was exactly a warning. It's more along the lines of "Listening on http://localhost:3000", which everyone is ok with.

[sebmarkbage]

What if this was in a shared npm package instead of inside React that only did this, and we depend on it? So that any library that has this concern can depend on that package and then it will get deduped and only executed once. That way you won't see many such warnings in the future.

[rauchg]

@aweary:

In some benchmarks (like this one) React is many times slower when run in dev mode.

Many people have incorrectly shipped a React dev version to production, and many others have run and published benchmarks against the dev version.

This is an attempt to avoid hitting that same wall time and time again.

[aweary]

I understand that it helps solve the problems of inaccurate benchmarks (been there), but this approach doesn't just target benchmarks or incorrectly shipped builds (which we try to handle already); it will target all cases where the development build is being used. The common case being developers just building their applications.

It feels like the wrong approach to address corner cases at the expense of the common case.

[gaearon]

We decided not to do it for now.

[gaearon]

(But might revisit if DevTools don't help enough. In our experience they've made people much more aware of the issue, and we sell them on DevTools from the first time they use React.)