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

options.jsonify is always forced to true #17

Closed
Venryx opened this issue Nov 11, 2019 · 2 comments · Fixed by #19
Closed

options.jsonify is always forced to true #17

Venryx opened this issue Nov 11, 2019 · 2 comments · Fixed by #19
Labels
kind: bug Something isn't working properly or is fixed

Comments

@Venryx
Copy link

Venryx commented Nov 11, 2019

The code in index.js has a bug: https://github.com/agilgur5/mst-persist/blob/master/src/index.ts

Look closely at the highlighted usages:

Note this line:

if (!jsonify) { jsonify = true } // default to true like mobx-persist

And note that there is no line after that which sets the jsonify variable's value.

Basically, the line above forces jsonify to always be true, even if the user explicitly sets it to false.

You probably meant this instead:

if (jsonify == null) { jsonify = true } // default to true like mobx-persist
@agilgur5
Copy link
Owner

agilgur5 commented Nov 11, 2019

Ah, poop, well that's a dumb mistake 😅 . Thanks for catching this and filing the issue @Venryx !

You probably meant this instead:

if (jsonify == null) { jsonify = true } // default to true like mobx-persist

Actually, I meant undefined, since if jsonify is not specified, it'll be undefined, not null. So that would look like:

if (typeof jsonify === 'undefined') { jsonify = true } // default to true like mobx-persist

At some point I just had jsonify = true as a default arg in the destructuring, and then once I wrote the somewhat complex (and later even more in #15) defaults logic for the storage config, I thought it would make sense to do all defaults outside of destructuring. That clearly invited some bugs 😖 .

=== undefined is meant to replicate how default args work, but I think I'll just make it a default arg instead to fix as I'm already using one for Transforms anyway and, as I found out during its implementation, default args make for better typings too.

@agilgur5
Copy link
Owner

Fix has been released in v0.1.2

@agilgur5 agilgur5 added the kind: bug Something isn't working properly or is fixed label Feb 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug Something isn't working properly or is fixed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants