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

don't omit message in production #172

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

iloveitaly
Copy link

It was very unexpected that the production version of the library is different than the development version.

It's very helpful to have the exact invariant message in the exception for easy debugging through tools like sentry.

Curious your thoughts here! I can fix up the tests if you are open to making this change.

@mykeels
Copy link

mykeels commented Feb 8, 2023

I agree, but I think this would be a breaking change for all the other systems using this.

Perhaps, adding an optional parameter to decide whether or not to hide error messages will be helpful, such as:

function invariant(condition, message, { hideErrorInfo = process.env.NODE_ENV === "production" } = {}) {
  if (hideErrorInfo) {
    throw new Error(prefix);
  }
}

This will ensure that the current interface of invariant(condition, message) continues to work as expected, while:

invariant(condition, message, { hideErrorInfo: false })

will ensure the actual error is thrown when necessary.

@iloveitaly
Copy link
Author

What do you think about a y.x.x bump to indicate a breaking change?

@BrianHung
Copy link

Also would like this PR since in sentry for production builds, I'm only seeing "Invariant failed" which doesn't help with developer debugging.

@iloveitaly
Copy link
Author

@BrianHung Yup, that's exactly my issue as well. Makes it harder to track down what went wrong.

@iloveitaly
Copy link
Author

@alexreardon curious if you have any thoughts here! Would love to help get this improvement in.

@kentcdodds
Copy link

This explains a lot of frustration. Please merge.

@iloveitaly
Copy link
Author

What do you think about a y.x.x bump to indicate a breaking change?

@mykeels what do you think about this?

@mykeels
Copy link

mykeels commented Sep 15, 2023

Yes please

@kentcdodds
Copy link

I've released my own version of this library with some improvements: https://github.com/epicweb-dev/invariant

@alexreardon
Copy link
Owner

I am open to this. I think that different folks have different needs. For a lot of frontend usages, the ideal is to ship as little code as possible. For backends, having verbose error messages is ideal. Also, some clients might want the verbose messages.

I'm open to ideas on how best to solve these needs for various consumers needs in a way the plays nicely with bundlers

@alexreardon
Copy link
Owner

Maybe the answer is not to be clever, but have one export for each variant. Could even use an entry point for extreme bundle size safety

import invariant from "tiny-invariant/keep-messages"

(For example)

@assertnotnull
Copy link

I join the discussion here. I had to spend some time wondering why my message is not printing in production with only a generic error message.

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

Successfully merging this pull request may close these issues.

None yet

6 participants