conf icon indicating copy to clipboard operation
conf copied to clipboard

Add support for setting/getting Date objects

Open sbencoding opened this issue 5 years ago • 11 comments

Implementing a new feature described in issue https://github.com/sindresorhus/electron-store/issues/18.
Type information is stored under ${INTERNAL_KEY}.type as suggested, the get method checks if there's a type key present, if there's it converts the value to the type before returning it.
The set methods checks if the value passed in is an instance of Date, if it is it stores it as an object like:

key: {
    __internal__.type: 'Date',
    val: valuePassedIn
}

I've also added a test to test.js to test for the setting and getting Date objects Let me know if there's anything I could change. For example I chose to include type information only for Date objects, but it might be better to store type information for every key-value pair to be more consistent, but it would also make the object and the file larger.

sbencoding avatar Sep 10 '19 05:09 sbencoding

The travis CI tests/builds are passing, but node v12 on Windows failed to download node.js v12, so the code should be fine I think.

sbencoding avatar Sep 10 '19 06:09 sbencoding

This needs to be documented in the readme and index.d.ts. Also make sure that $ npm test passes locally.

sindresorhus avatar Sep 10 '19 06:09 sindresorhus

Hi, sorry I've been busy yesterday, but here are the modifications.
I've updated the documentation in readme.md and in index.d.ts, I've added typescript tests for the Date serialization.
I've also refactored the code a bit, to handle Date objects within nested objects, moved serialization to the getter and setter directly instead of the user facing get and set method.
Adding new types/objects is also easier now, because the necessary information is stored in an array, instead of writing an if condition for every new type.
Let me know what you think

sbencoding avatar Sep 11 '19 18:09 sbencoding

Any updates on merging this functionality? Would be great to have a simple way to store and retrieve dates.

delewis13 avatar Feb 10 '20 00:02 delewis13

@delewis13 I've implemented the requested changes, but I didn't want to update the PR, because there are still questions, namely:

  • Circular objects In the review there's a request to handle circular objects, but even if I handle them JSON.serialize() just throws and exception when it detects one, so I'm not sure what to do here
  • Usage of while loops I've commented on the review explaining my thought process, but there might be other, better ways instead of using the while loop I haven't thought about.

Other than that the rest of the requested changes are implemented

sbencoding avatar Feb 13 '20 20:02 sbencoding

Hi @sbencoding, there are other modules that can handle serialization and deserialization of circular objects automatically for you, such as flatted, maybe this is what Sindre had in mind...

papb avatar Feb 14 '20 02:02 papb

Hey, sorry for the late response. I've implemented the serialization and deserialization of circular objects, as well as I've added some tests for it. Other requested changes are implemented as well. I'm not sure why the travis build fails tho....

  • On all windows builds xo complains about import order in test.js, this doesn't happen for OSX or Linux or my local machine (Linux).
  • OSX with nodejs v12 fails the both of the watch option tests. This doesn't happen for v10, and on the linux tests.

sbencoding avatar Mar 15 '20 14:03 sbencoding

I've implemented most of the requested changes, I'm just going to re-work a bit the addition of extra types by the consumer program tomorrow, and then I'm going push the new changes.

I'm really confused about what I should do with circular objects now, but that's explained in a comment in the code review. I'd be happy to implement another approach, if I'm missing the point of this feature.

sbencoding avatar Apr 07 '20 22:04 sbencoding

I've implemented the requested changes, now the build only fails for node v12 on macOS during the 2 .watch() tests.

Instead of directly updating the extraTypes array of a Conf instance, I chose to implement the addition of extra types through the options argument that's passed to the constructor.

Let me know if there's anything else to change.

sbencoding avatar Apr 08 '20 13:04 sbencoding

There are still some unresolved inline comments. Also make sure that the docs in the readme is in sync with index.d.ts and the other way.

sindresorhus avatar Apr 23 '20 07:04 sindresorhus

I've updated the example and double-checked to docs to reflect each other as much as possible, let me know if there's anything else to change/update.

sbencoding avatar Apr 23 '20 17:04 sbencoding

I appreciate all the work done here, but I unfortunately don't think this is the optimal solution. Closing in favor of #154.

sindresorhus avatar Feb 28 '24 07:02 sindresorhus