refinement doesn't run type checks in production 😱

[davetapley]

Bug report

• [x] I've checked documentation and searched for existing issues
• [x] I've made sure my project is based on the latest MST version
• [x] Fork this code sandbox or another minimal reproduction.

Sandbox link or minimal reproduction code

1. Open: https://codesandbox.io/s/mst-refinement-dev-mode-xk0w34?file=/.env
2. Press 'Add' button

Describe the expected behavior

Crash with unhandled exception:

[mobx-state-tree] Error while converting `{"title":""}` to `Todo`:

because I modified Todo.js to be:

types.refinement(types.string, (str) => str.length > 0),

Describe the observed behavior

It doesn't crash.

Note that if you delete the .env file I added (putting it back in to development mode) the exception is raised.

[davetapley]

Commit and code linked below makes me presume this is working as intended, and has been this way since v3.0.1.

Which, okay, but there's no mention of it in the docs for refinement, and having a feature which intentionally changes semantics only in production seems like a bad idea to me (even if it is added to docs) 😬

... that said I can only imagine 'fixing the bug' could cause big trouble for existing code bases, since any refinement which isn't exercised in tests would start throwing in production 😱

... but I also wonder how many unknown bugs are out there because lack of throw in production. In this case a bug would only surface if you later tried to do something with a refinement which assumes that the refinement is upheld by create in production (which is what led me to find this issue, since I added a refinement to ensure a NaN couldn't be put in to a types.number, see: #1960).

---

First appeared in v3.0.1: https://github.com/mobxjs/mobx-state-tree/commit/931976f441f80bd0aa2441fd6c64f9313b247dbb, and now implemented as:

https://github.com/mobxjs/mobx-state-tree/blob/4a6a9a5f5bd02a779f395960cb8aef44d6e1d03f/packages/mobx-state-tree/src/utils.ts#L407-L420

[coolsoftwaretyler]

This recently affected my codebase as well. We had API data that changed and violated the types.

In development, we saw errors being thrown. But we did not see errors in prod.

It's a nice reversal of "it works on my machine". Our users' environment was somewhat more resilient (although it introduced subtle errors with regard to missing data).

The big problem for us was that we thought we broke something in a commit that happened after the most recent production deploy, when the issue was in an external dependency (a breaking change in API response). Spent a lot of time looking for regressions in unrelated MRs.

Overall, I agree that it's a problem to change semantics based on the environment. I also imagine changing that behavior would be dangerous and require a major version bump.

I feel like I read somewhere that this is a performance concern - it's expensive to do runtime type checking.

Perhaps there's an option to run MST in "strict" mode, which would funnily behave like development mode.

If there's interest from the maintainers at exploring solutions, I'd be happy to contribute some time and effort here.

[coolsoftwaretyler]

Another idea that comes to mind: never throw errors, not even in development. But in development, emit warnings or logs about issues.

That way, teams wouldn't write error-handling logic and be surprised when exceptions stop getting thrown in production. The development environment would be noisier, but the behavior would stay consistent.

[coolsoftwaretyler]

It's been a while since this issue has seen movement, but I'm of the opinion that this is worth exploring. I'm going to leave it as an issue for now, but I'm open to:

1. Moving to discussions since it might be a breaking change
2. Deciding not to make this change, since it'll break expected behavior
3. Deciding not to make this change if it doesn't align well with the expected behavior anyway
4. Updating documentation to help clarify expectations?
5. Somethin' else?