Skip to content
This repository has been archived by the owner on Dec 31, 2020. It is now read-only.

Fix #806 #829

Merged
merged 2 commits into from Feb 6, 2020
Merged

Fix #806 #829

merged 2 commits into from Feb 6, 2020

Conversation

Bnaya
Copy link
Member

@Bnaya Bnaya commented Feb 6, 2020

No description provided.

@coveralls
Copy link

coveralls commented Feb 6, 2020

Coverage Status

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

src/observerClass.ts Outdated Show resolved Hide resolved
test/issue806.test.js Outdated Show resolved Hide resolved
@@ -153,7 +153,17 @@ function makeObservableProp(target: any, propName: string): void {
configurable: true,
enumerable: true,
get: function() {
let prevReadState = false;

if (_allowStateReadsStart && _allowStateReadsEnd) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mweststrate should we import * as mobx or this null check is enough?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nope. this should work fine

render() {
return <span>
{this.props.a}
<Issue806Component2 propA={this.props.a} propB={this.props.b} />
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why it was needed, but i've added these 😅

@Bnaya Bnaya merged commit 92b3cd2 into master Feb 6, 2020
@Bnaya Bnaya deleted the fix-#806 branch February 6, 2020 14:11
ynejati added a commit to ynejati/mobx-react that referenced this pull request Feb 6, 2020
@danielkcz
Copy link
Contributor

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

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
Copy link
Member Author

Bnaya commented Feb 6, 2020

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

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

danielkcz pushed a commit that referenced this pull request Feb 7, 2020
* Issue #824: convert tests from JavaScript to TypeScript [WIP]

* More conversion of tests and resolution of errors
Upgrading snapshots

* Resolve typescript and react linter issues

* Code feedback

* Updating testing libraries and using more optional chaining

* Resolve conflicts, convert PR #829, and make pretty

* Update yarn lock after resolving conflicts with master branch

* Linter and TSC fixes
@github-actions github-actions bot mentioned this pull request Oct 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants