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

Optional options is no longer optional with v3.2.0 #91

Closed
iamchucky opened this issue Oct 17, 2017 · 7 comments
Closed

Optional options is no longer optional with v3.2.0 #91

iamchucky opened this issue Oct 17, 2017 · 7 comments

Comments

@iamchucky
Copy link

Due to the changes in v3.2.0, there are bugs when calling storage.set(key, json, [options], callback) without options

Uncaught Exception:
TypeError: Cannot read property 'dataPath' of undefined
    at Object.exports.set

It throws error when trying to access options.dataPath at https://github.com/electron-userland/electron-json-storage/blob/master/lib/storage.js#L256

I think we need to ensure options is set to {} at https://github.com/electron-userland/electron-json-storage/blob/master/lib/storage.js#L251 and make sure all other similar methods do the same thing, like .get and .has .remove .clear

@jviotti
Copy link
Member

jviotti commented Oct 18, 2017

Hi @iamchucky,

Can you show me an example that reproduces the error? There are various cases on the test suite where we use the .set() function without options (i.e. https://github.com/electron-userland/electron-json-storage/blob/master/tests/storage.spec.js#L198), but maybe I'm missing something?

@iamchucky
Copy link
Author

I was not passing callback either:

storage.set('settings', data);

example like above would work before 3.2.0 but throws error with 3.2.0

But now I realize that I should probably pass the callback function in as it is a required argument according to the documentation.

@jviotti
Copy link
Member

jviotti commented Oct 19, 2017

Ah, that makes sense. I think it should still be easy to fix. I'll prepare a PR later today.

jviotti added a commit that referenced this issue Oct 19, 2017
This allows users to call the async functions in a fire and forget
fashion.

Fixes: #91
Signed-off-by: Juan Cruz Viotti <jv@jviotti.com>
@jviotti
Copy link
Member

jviotti commented Oct 19, 2017

@iamchucky Can you give #93 a go?

@jviotti
Copy link
Member

jviotti commented Oct 19, 2017

Check v4.0.1

@iamchucky
Copy link
Author

iamchucky commented Oct 20, 2017

I don't think #93 fixes this particular issue, when call storage.set('settings', data); without both options and callback, options is undefined, therefore throws at

dataPath: options.dataPath

Uncaught Exception:
TypeError: Cannot read property 'dataPath' of undefined
    at Object.exports.set

jviotti added a commit that referenced this issue Oct 20, 2017
Fixes: #91
Signed-off-by: Juan Cruz Viotti <jv@jviotti.com>
@jviotti
Copy link
Member

jviotti commented Oct 20, 2017

Good point. What about v4.0.2?

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

No branches or pull requests

2 participants