-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
requiresReaction
in options of computed
does not throw as indicated by the documentation
#3618
Comments
I noticed that this behavior was changed in #2998 possibly intentionally. But as I mentioned in #2417 (comment) previously, we in Canva find it hard to enforce restrictions if they don't actually throw, because developers don't look at console all the time when nothing goes wrong, and it is hard to track all the requirements across a large codebase. So we would prefer there to be a way to enforce the restrictions. If it is intended to make |
There is also another potential use case of An example as below: let _blobUrl: string | undefined;
const blobUrl = mobx.computed(() => {
// some check
if (_blobUrl != null) {
URL.revokeObjectURL(_blobUrl);
}
// generate a blob
_blobUrl = URL.createObjectURL(blob);
return _blobUrl;
});
mobx.onBecomeUnobserved(blobUrl, () => {
if (_blobUrl != null) {
URL.revokeObjectURL(_blobUrl);
_blobUrl = undefined;
}
}); However, if you call |
Do I follow correctly it actually reports, so it's only about warn vs throw? As a workaround you can patch I would like to keep it consistent. I also prefer if errors are sound and basically non-ignorable, but one thing to consider is that these checks are dev only - conceptully I think it's a bit weird if something is considered an error in dev, but not on prod. IIRC React also just logs in most of the "suspicious" situations. Also errors are often thrown from async contexts, so they end up only in the console anyway. Maybe we could introduce |
There are many warnings that should be fixed, like React key warnings, that are not errors to not deviate from production behavior, as otherwise bug reproductions can become extremely hard. If checking warnings is not part of the developers typical habits before putting up a PR, I'd indeed solve this as the infra level of your product, and raise on-screen alerts on any logged warning/error you believe must always be fixed in your company. That being said, I think this flag was actually intended to throw even in production, and that the bug here is that it was after the So I'd be supportive of moving this back to throw, but move the check outside DEV. My concern however is potentially breaking production systems, so I'm wondering if this should be a major change? Advocating for the devil, the spec says already it throws, so it is probably fine as patch. |
While it is true that lack of key in list is a React warning that should be fixed, but this mistake is generally very local, and can easily be caught during self-review and review, as well as via static linting. However, it is not the case for things like Also warnings don't always carry a stack (as in Firefox), and sometimes don't even carry the location (as in Safari), which can make it harder to locate the source of the issue given the remote natural of the issue.
That is an interesting suggestion we will consider. But as I mentioned above, other warnings are not generally an issue given their local nature in code, and we haven't been having problem with preventing them, so this issue is pretty much specific to the mobx ones. |
I would also argue that
is not a very sustainable way to do it in long term. You already don't consider behavior change switching from throwing to warning to be a major change, how can we ensure that the warning message wouldn't get accidentally reworded in a way we fail to capture it after a patch upgrade, and without even the change log mentioning it given it's trivial looking? |
The change itself wasn't breaking, it only changed the dev experience.
https://github.com/mobxjs/mobx/releases/tag/mobx%406.3.3 |
The document says:
Intended outcome:
It should throw, because it is not called within a reactive context.
Actual outcome:
It does not throw.
How to reproduce the issue:
Just run the code above.
Versions
MobX 6.
The text was updated successfully, but these errors were encountered: