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

refinement doesn't run type checks in production 😱 #1962

Open
3 tasks done
davetapley opened this issue Sep 19, 2022 · 4 comments
Open
3 tasks done

refinement doesn't run type checks in production 😱 #1962

davetapley opened this issue Sep 19, 2022 · 4 comments
Labels
brainstorming/wild idea Brainstorming / Wild idea breaking change Breaking change help/PR welcome Help/Pull request from contributors to fix the issue is welcome level: intermediate not a bug Seems like a bug, but actually it is not

Comments

@davetapley
Copy link
Contributor

Bug report

  • I've checked documentation and searched for existing issues
  • I've made sure my project is based on the latest MST version
  • 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
Copy link
Contributor Author

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: 931976f, and now implemented as:

export function isTypeCheckingEnabled() {
return (
devMode() ||
(typeof process !== "undefined" && process.env && process.env.ENABLE_TYPE_CHECK === "true")
)
}
/**
* @internal
* @hidden
*/
export function devMode() {
return process.env.NODE_ENV !== "production"
}

@coolsoftwaretyler
Copy link
Collaborator

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
Copy link
Collaborator

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 coolsoftwaretyler added brainstorming/wild idea Brainstorming / Wild idea breaking change Breaking change help/PR welcome Help/Pull request from contributors to fix the issue is welcome level: intermediate not a bug Seems like a bug, but actually it is not labels Jun 25, 2023
@coolsoftwaretyler
Copy link
Collaborator

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
brainstorming/wild idea Brainstorming / Wild idea breaking change Breaking change help/PR welcome Help/Pull request from contributors to fix the issue is welcome level: intermediate not a bug Seems like a bug, but actually it is not
Projects
None yet
Development

No branches or pull requests

2 participants