Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Development diagnostics should be dropped from production version #35

Open
Satyam opened this issue Apr 5, 2018 · 4 comments
Open

Development diagnostics should be dropped from production version #35

Satyam opened this issue Apr 5, 2018 · 4 comments
Labels

Comments

@Satyam
Copy link
Contributor

Satyam commented Apr 5, 2018

Diagnostics like this one are not helpful in production versions. It is better to drop them conditionally:

if (process.env.NODE_ENV !== 'production') {
 // diagnostics
}

When minimized for production by the final developer, it will be dropped as it is dead code.
The version of this utility published on npm should not be packed in production mode so the final developer gets this diagnostics.

@gioragutt
Copy link
Contributor

@Satyam the link you provided does not show the problem you addressed. Could you fix it so that we could see exactly what you mean?

@Satyam
Copy link
Contributor Author

Satyam commented Apr 6, 2018

You can see it at work in Facebook's own prop-types where the whole code for the property checker is enclosed in an if (process.env.NODE_ENV !== 'production') { block. In that way, the diagnostic, quite useful in development and testing is dropped as dead_code when Webpack runs Uglify for the production version.

Cases like the code I linked to might be more useful like this:

if (process.env.NODE_ENV !== 'production') {
  const hasPayload = has(mapped, 'payload')
  const hasMeta = has(mapped, 'meta')
  const hasMore = ownKeys(mapped).length > 2
  if (hasPayload !== hasMeta || hasPayload && hasMeta && hasMore) {
    console.error('Full mapper should return both meta and payload, and only these two.')
  }
}

Notice also that the error has been changed to a console.error which allows the code to continue to run, thus printing all the errors at once and allowing the developer to fix them all in one go. This is just an optional way to handle it. prop-types does it this way, but throwing an error is totally valid, I just thought to mention the alternative.

@gioragutt
Copy link
Contributor

Oh I get it. So your proposal is to add these kind of checks.

@Satyam
Copy link
Contributor Author

Satyam commented Apr 6, 2018

It is the best way to provide them without concern as to how they may affect the performance/size of the end product. With this trick, they will be gone.

As I mentioned in my first post, it is the end-developer who should create the bundle in production mode and strip the diagnostics. The library creator should publish it with those warnings still on,for the benefit of the final developer. Utilities like create-react-app automatically do this, from their docs:

There is also a special built-in environment variable called NODE_ENV. You can read it from process.env.NODE_ENV. When you run npm start, it is always equal to 'development', when you run npm test it is always equal to 'test', and when you run npm run build to make a production bundle, it is always equal to 'production'. You cannot override NODE_ENV manually. This prevents developers from accidentally deploying a slow development build to production.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants