search

mobxjs / mobx-react

React bindings for MobX
https://mobx.js.org/react-integration.html
MIT License
4.85k stars 349 forks source link

Fix #806 #829

Closed Bnaya closed 4 years ago

coveralls commented 4 years ago

Coverage Status

Coverage increased (+0.07%) to 95.467% when pulling 96d9f901951861e09423c695da9a550e2df37b01 on fix-#806 into d6d1001af0b7049035ddee538bd5a1c0e01e5678 on master.

danielkcz commented 4 years ago

I don't follow why to bump peer dep. It's not like it will force a consumer to install that specific version, only warn about it.

Besides, requiring a specific version feels like sort of a breaking change to me. The code has to work with X.0.0 versions.

mweststrate commented 4 years ago

I don't follow why to bump peer dep. It's not like it will force a consumer to install that specific version, only warn about it.

Yeah that is the kind of the point, if you bump mobx-react we want ideally want folks to bump mobx is as well to not run into #806

Besides, requiring a specific version feels like sort of a breaking change to me. The code has to work with X.0.0 versions.

It still does, but in that case you will still suffer from the false positive from #806

Bnaya commented 4 years ago

Btw i think we have broken code. in the past, Weve added new apis to mobx, used them without null checks in mobx-react, and without peer bump. I woudnt mind revert the change and maybe add null checks + dev time warnings

danielkcz commented 4 years ago

It's already published, but feel free to improve for the next version.