mobx-react 6.2.1 introduced a breaking change with batched updates.

[melnikov-s]

Intended outcome

This change to be released as 7.x

Actual outcome

Released as a minor version

How to reproduce the issue

https://codesandbox.io/s/minimal-mobx-react-project-nxctz?file=/index.js

our app broke when we upgraded to mobx-react 6.2.2 from 6.1.8 . This was due to a breaking change in how batched renders are handled. PR here: #787 . This is was then fixed with import 'mobx-react/batchingForReactDom' but we were not expecting a breaking change with a minor version bump (it's also a tricky issue to diagnose).

I noticed a detailed write up of what can happen without batching here: #787 (comment) so perhaps this was released by mistake?

[danielkcz]

You are partially right, it is sort of a breaking change, but it doesn't require any changes to your existing code. All you need to do is to follow a friendly warning. As such I don't think it warrants for next major.

Can you tell me if that warning wasn't clear enough or why did you decide to debug a problem first before following the advice? Can we improve that warning to make it more clear?

[melnikov-s]

Following the warning requires making changes to existing code does it not?

Either way, we did not see any warning, I'm not sure why (prod vs dev?). I do not see a warning in the code sandbox either.

[mweststrate]

We could revert and republish as major? This package doesnt have any downstream packages afaik so there are no complications to that?

…

On Fri, 1 May 2020, 17:33 Daniel K., ***@***.***> wrote: You are partially right, it is sort of a breaking change, but it doesn't require any changes to your existing code. All you need to do is follow a friendly warning. Can you tell me if that warning wasn't clear enough or why did you decide to debug a problem first before following it? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#859 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAN4NBA4U3XCRZCYW5D6E4DRPL2UVANCNFSM4MXGJEVQ> .

[danielkcz]

Following the warning requires making changes to existing code does it not?

It's about adding one line import somewhere to the top of the app code. It's arguable if that's "change to existing code". It's not like you need to change every observable component :)

Either way, we did not see any warning, I'm not sure why (prod vs dev?). I do not see a warning in the code sandbox either.

Interesting. CodeSandbox is understandable as they are running that code in production mode sadly. So you mean you don't have any QA in place that would catch the app not working before dep updates go to production? I mean this is open-source. Expecting 100% reliability is certainly risky.

We could revert and republish as major?

I don't know. I still don't feel like such a change warrants for the major. This is the first case, nobody else seemed to have a problem so far. Let's keep this open and wait in my opinion.

[melnikov-s]

It's about adding one line import somewhere to the top of the app code.

That's by definition a change to existing code.

I mean this is open-source. Expecting 100% reliability is certainly risky.

I'm not expecting that. I'm just reporting a violation of semver so that it can be addressed or at least documented if anyone else runs into this issue.

So you mean you don't have any QA in place that would catch the app not working before dep updates go to production?

Didn't make it into production broke the build first, we mistakenly used the ^ (^6.1.8) instead of hard-coding it so the build picked up a newer version.

[danielkcz]

Didn't make it into production broke the build first, we mistakenly used the ^ (^6.1.8) instead of hard-coding it so the build picked up a newer version.

That shouldn't matter if you use lock files (yarn.lock or package-lock.json) which requires you to explicitly upgrade versions.

You are saying it broke the build, how? Are you hiding warnings from logs? If you have tests, those warnings should appear in there. I would like to find a way to improve this without immediately jumping for next major.

[melnikov-s]

I'm not sure that the specifics of our build environment is relevant here, yes it could and should be improved to catch these type of situations and the fact that it didn't is our failing. We should not trust a human procedure like following semver to be 100% reliable. That doesn't change the fact that the 6.2.1 is a minor release that contains a breaking change.

if mobx-react follows semver this change should be reverted.

[danielkcz]

I am definitely not trying to blame you for having a bad system in a place. I am just trying to figure what can we do better to inform about this change without going for a major right away.

I know, we should adhere to semver, heard you the first time. However, it's already out there and it doesn't seem to bother anyone else. It's probably not that serious how you are trying to sell it here. No offense :)

Reverting and going major for such a single insignificant change could only confuse other people. There is also a lot of mentions of V6 in various places and docs which makes such a major release non-trivial job. We are not react-testing-library to have major release almost every other week 😆

[melnikov-s]

I'm not trying to sell it for anything other then what it is. I can't speak for other applications but for us it caused a breakage. It will only really be noticed if there's an exception in a child component that's not supposed to render, if there's not then it will just result in a wasted render and those are not easy to track.

I am just trying to figure what can we do better to inform about this change without going for a major right away.

There's a snippet in the docs about it, that's good. Maybe add how it was opted in by default before 6.2.1 and now you have to opt-in yourself? If you can somehow detect react-dom/react-native and opt into by default for 6.3 then you'll fix the breaking change, keep the feature and not require a major. Don't know if that's possible though.

[danielkcz]

Thanks for the feedback, I will add some note about the version in docs and hopefully, it will be enough for now.

Btw, the change was introduced in 6.2.0. The 6.2.1 just had a small fix for that.

If you can somehow detect react-dom/react-native and opt into by default for 6.3 then you'll fix the breaking change, keep the feature and not require a major.

Yea, unfortunately, I am not aware of any good approach for that. That's why we went this way. At the moment you have require('react-dom') in the code, the bundler will pick up and tries to find it and fails when on react-native or vice versa. Nothing like "optional" dependency exist for this matter.

[vkrol]

I completely agree with @melnikov-s that it was supposed to be a major version.

[melnikov-s]

Btw, the change was introduced in 6.2.0. The 6.2.1 just had a small fix for that.

Really? Because in the code sandbox changing the version to 6.2.0 (and refreshing) does not produce the issue, while with 6.2.1 it does.

[danielkcz]

That's because 6.2.0 accidentally still had a direct call to react-dom scheduler and thus breaking it for react-native people (#852). The 6.2.1 removed it. I guess technically breaking change happened with patch version then 😎

Let's leave this for now. Unless I see more people having a problem with this, I am very reluctant about going for the major version here.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.