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.

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

Refactor toJS #1699

Merged
merged 4 commits into from Sep 28, 2018
Merged

Refactor toJS #1699

merged 4 commits into from Sep 28, 2018

Conversation

wangyiz4262
Copy link
Contributor

@wangyiz4262 wangyiz4262 commented Aug 27, 2018

PR checklist:

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

toJS will recursively convert an object containing Mobx observable value into a plain object

@coveralls
Copy link

coveralls commented Aug 27, 2018

Coverage Status

Coverage increased (+0.1%) to 93.717% when pulling c52b25e on wangyiz4262:refactor-toJS into ff78f69 on mobxjs:master.

@mweststrate
Copy link
Member

Hey @wangyiz4262, I can't approve the change as-is, as this would break with the current behavior, producing different results. I'm fine though if this behavior is applied when a special flag is applied, for example: { recurseEverything: true}. In that case make sure that there are unit tests covering both the flag enabled and disabled.

@wangyiz4262
Copy link
Contributor Author

@mweststrate, thanks for the advice, will do!

@wangyiz4262 wangyiz4262 changed the title Refactor toJS WIP: Refactor toJS Sep 15, 2018
@wangyiz4262 wangyiz4262 changed the title WIP: Refactor toJS Refactor toJS Sep 20, 2018
@wangyiz4262
Copy link
Contributor Author

Hi @mweststrate, I have added an optional flag as you suggested. Please check it out and merge if it is OK. Thanks!

Copy link
Member

@mweststrate mweststrate left a comment

Choose a reason for hiding this comment

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

Almost there 👍 left some questions / comments

src/api/tojs.ts Outdated Show resolved Hide resolved
src/api/tojs.ts Show resolved Hide resolved
test/base/tojs.js Outdated Show resolved Hide resolved
src/api/tojs.ts Show resolved Hide resolved
@wangyiz4262
Copy link
Contributor Author

Hi @mweststrate All comments have been addressed. And changes are also available for mobx4 in PR #1733

@mweststrate mweststrate merged commit be74b5f into mobxjs:master Sep 28, 2018
@mweststrate
Copy link
Member

Thanks, merged! Will be part of next release

@mweststrate
Copy link
Member

Released as 5.5.1. Sorry for the late release! Missed that a PR was pending for release

@stnwk
Copy link

stnwk commented Oct 18, 2018

Thanks! :)

@wangyiz4262 wangyiz4262 deleted the refactor-toJS branch October 23, 2018 02:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants