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.

Sign up for GitHub

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

Jump to bottom

Add check for nullable source on toJS() #1783

Merged
merged 2 commits into from
Oct 22, 2018
Merged

Add check for nullable source on toJS() #1783

merged 2 commits into from
Oct 22, 2018

Conversation

wangyiz4262
Copy link
Contributor

@wangyiz4262 wangyiz4262 commented Oct 22, 2018 •
edited
Loading

Addressing bug discovered by @y4ure in issue #1557

PR checklist:

  • Added unit tests
  • Verified that there is no significant performance drop (npm run perf)

@wangyiz4262 wangyiz4262 changed the title Add check for nullable source Add check for nullable source on toJS() Oct 22, 2018
@coveralls
Copy link

Coverage Status

Coverage remained the same at 93.717% when pulling 51b7885 on wangyiz4262:patch-1 into 9c89b50 on mobxjs:master.

@mobxjs mobxjs deleted a comment from wangyiz4262 Oct 22, 2018
@wangyiz4262 wangyiz4262 mentioned this pull request Oct 22, 2018
Closed
@capaj capaj added the 🐛 bug label Oct 22, 2018
capaj
capaj approved these changes Oct 22, 2018
View reviewed changes
Copy link
Member

@capaj capaj left a comment

Choose a reason for hiding this comment

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

LGTM

@capaj capaj merged commit 397c1a1 into mobxjs:master Oct 22, 2018
@wangyiz4262 wangyiz4262 deleted the patch-1 branch October 23, 2018 02:08
@mweststrate
Copy link
Member

Thanks for the PR and reviewing!

Released as 4.5.2 / 5.5.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ItamarShDev ItamarShDev ItamarShDev left review comments

@capaj capaj capaj approved these changes

Assignees
No one assigned
Labels
🐛 bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@wangyiz4262 @coveralls @mweststrate @ItamarShDev @capaj