Add support for setting/getting Date objects #89

sbencoding · 2019-09-10T05:52:04Z

Implementing a new feature described in issue sindresorhus/electron-store#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 · 2019-09-10T06:01:14Z

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.

sindresorhus · 2019-09-10T06:02:39Z

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

sbencoding · 2019-09-11T18:23:21Z

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

sindresorhus · 2019-09-22T10:57:16Z

index.js

+		this.extraTypes = [
+			{
+				name: 'Date',
+				instance: Date,


I think this should be a function too. We should not assume all future types can be detected through an instance check.

sindresorhus · 2019-09-22T10:57:25Z

index.js

+				name: 'Date',
+				instance: Date,
+				convertFrom: val => val.getTime(),
+				convertTo: val => new Date(val)


val => value (everywhere)

sindresorhus · 2019-09-22T10:58:09Z

index.js

+		stack.push(value);
+		while (stack.length > 0) {
+			const current = stack.pop();
+			Object.keys(current).forEach(currentItemKey => {


Use Object.entries() and for-of loop.

sindresorhus · 2019-09-22T10:58:46Z

readme.md

 #### .set(key, value)

 Set an item.

-The `value` must be JSON serializable. Trying to set the type `undefined`, `function`, or `symbol` will result in a TypeError.
+The `value` must be JSON serializable. Trying to set the type `undefined`, `function`, or `symbol` will result in a TypeError.  


Don't do unrelated changes.

sindresorhus · 2019-09-22T10:59:33Z

test.js

+	t.context.config.set('dateobj', {str: 'this is a string', date: new Date()});
+	const myDate = t.context.config.get('dateobj').date;
+	t.true(myDate instanceof Date);
+});


We need a lot more tests for this feature, including ensuring it handles circular objects, objects inside arrays inside objects (and circular for that too), and other things you can think of.

I've added more tests, but I'm not sure how to go about circular objects, becuase Conf.serialize, which is JSON.stringify by default, throws an exception is there's a circulation. So all I did for now is to detect if there's a circulation and stop handling that branch of the object, because it would cause an infinite loop, and cause the application to crash, but when Conf gets to serializing the object with JSON it still throws an error. I've also tested this behavior with string so it's not a problem with how the new feature handles extra types.

sindresorhus · 2019-09-22T11:01:44Z

index.d.ts

+	interface ExtraType<T> {
+		name: string,
+		instance: T,
+		convertFrom: (val: T) => string | number,


The return value here would be a good candidate for https://github.com/sindresorhus/type-fest/blob/eb110db80ccd74e8f5141cb52b3e99d3603399ce/source/basic.d.ts#L53

sindresorhus · 2019-09-22T11:01:53Z

index.d.ts

@@ -248,7 +256,7 @@ declare class Conf<T = any> implements Iterable<[keyof T, T[keyof T]]> {
 	Set an item.

 	@param key - You can use [dot-notation](https://github.com/sindresorhus/dot-prop) in a key to access nested properties.
-	@param value - Must be JSON serializable. Trying to set the type `undefined`, `function`, or `symbol` will result in a `TypeError`.
+	@param value - Must be JSON serializable. Trying to set the type `undefined`, `function`, or `symbol` will result in a `TypeError`. Additionally Conf automatically seralizes and deserializes `Date` objects


sindresorhus · 2019-09-22T11:27:29Z

index.d.ts

@@ -211,6 +218,7 @@ declare class Conf<T = any> implements Iterable<[keyof T, T[keyof T]]> {
 	store: T;
 	readonly path: string;
 	readonly size: number;
+	readonly extraTypes: Conf.ExtraType<any>[];


Should not be any.

sindresorhus · 2019-09-22T11:27:52Z

index.js

+		// Sub-object handling of extra types
+		const stack = [];
+		stack.push(value);
+		while (stack.length > 0) {


Why are you using a while loop?

So in case of nested objects I want to reuse the same logic to serialize Date types, so there are two ways I can go about this. The first one is to use recursion, the other one is to use the iterative solution, which is the while loop.
I've decided to use the iterative solution over the recursive solution, because with really deeply nested objects, there's a possibility that the function could overflow the callstack.

sindresorhus · 2019-09-22T11:28:25Z

readme.md

+Currently supported objects are:
+- `Date` - serializes the Date to milliseconds, then back to a `Date` object when the getter is called
+
+If you want support for a new object/type please open a new issue for discussion.  


The docs in the readme and index.d.ts should be in sync.

delewis13 · 2020-02-10T00:12:06Z

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

sbencoding · 2020-02-13T20:14:26Z

@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

papb · 2020-02-14T02:09:00Z

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...

Convert dates when in a nested object Move conversion to store getter, setter

Add typescript tests for Date objects Add typescript definition of extraTypes array

Convert dates when in a nested object Move conversion to store getter, setter

Add typescript tests for Date objects Add typescript definition of extraTypes array

sbencoding · 2020-03-15T14:16:46Z

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.

sindresorhus · 2020-04-07T05:28:28Z

index.d.ts

@@ -1,9 +1,30 @@
 /// <reference types="node"/>
-import {JSONSchema} from 'json-schema-typed';
+import { JSONSchema } from 'json-schema-typed';


Don't do unrelated style changes. Applies in many places. If you upgrade the XO dependency and run npx xo --fix these things will be fixed for you.

sindresorhus · 2020-04-07T05:28:45Z

index.d.ts

+	/**
+	 * Matches a JSON object
+	 */


Suggested change

/**

* Matches a JSON object

*/

/**

Matches a JSON object.

*/

Applies everywhere.

sindresorhus · 2020-04-07T05:29:47Z

index.d.ts

@@ -225,6 +268,10 @@ declare class Conf<T = any> implements Iterable<[keyof T, T[keyof T]]> {
 	store: T;
 	readonly path: string;
 	readonly size: number;
+	/**
+	 * Conf can automatically (de)serialize some objects/types, for more information please visit [Supported Types](https://https://github.com/sindresorhus/conf#Supported-Types)


The readme and index.d.ts docs should be in sync. That means the linked to text should be included inline here.

sindresorhus · 2020-04-07T05:30:21Z

index.js

+		let circularReferenceCounter = 1;
+		while (stack.length > 0) {
+			const {current, parent, key} = stack.pop();
+			if (objectSeenList.indexOf(current) === -1) {


Suggested change

if (objectSeenList.indexOf(current) === -1) {

if (!objectSeenList.includes(current)) {

sindresorhus · 2020-04-07T05:31:05Z

index.js

+		}
+
+		return value;
+	}


This method is pretty large. I would look into splitting it into multiple methods and maybe something here is generic enough to be moved out into a generic utility function. Same with _deserializeExtraTypes.

sindresorhus · 2020-04-07T05:34:40Z

index.js

+				// seenList.indexOf(current) is the object it's pointing to
+				const referenceTarget = objectSeenList[objectSeenList.indexOf(current)];
+				parent[key] = {
+					[TYPE_KEY]: 'circularObject',


I don't really see why you need the circularObject thing. Circular objects can simply be detected by comparing references. I think this logic could be simplified a lot.

See how it's done here: https://github.com/sindresorhus/serialize-error/blob/master/index.js and here: https://github.com/sindresorhus/map-obj/blob/master/index.js

The circularObject type signals to the deserialize function that it was a circular object, so it can lookup the referenceNumber set in the value of the object, and set it to the object it was referencing to before serialization.

I might have misunderstood this problem. I made it so that when deserializing the circulation is reconstructed, so you're going to end up with the same circular object after getting it from the store.
So should I just completely remove circulation like the destroyCircular function does in the first example you've provided, or is there an easier way to reconstruct the circulation?

sindresorhus · 2020-04-07T05:35:25Z

test.js

+	t.true(getCircular.c1.myDate instanceof Date);
+	t.true(getCircular.c2.myDate instanceof Date);
+});
+


There needs to be some tests for other types, just to ensure it works in general with custom types and other types than Date.

sindresorhus · 2020-04-07T05:35:43Z

index.js

+					console.log(referenceTarget[CIRCULAR_REF_KEY]);
+				}
+
+				console.log('skipping pontentially circulation');


Modules should not log to the console.

Yeah, I'm very, very sorry about this, sometimes I just add some log statements for testing, and I always try to keep in mind to remove them, but it seems like I've forgotten this time.

sindresorhus · 2020-04-07T05:36:20Z

readme.md

@@ -356,6 +358,15 @@ conf.store = {



You need to document how to use the custom deserializer/serializer.

sbencoding · 2020-04-07T22:04:28Z

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.

…structor

sbencoding · 2020-04-08T13:17:23Z

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.

sindresorhus · 2020-04-23T07:01:12Z

readme.md

+- It must return the type that is defined.
+
+#### Example
+The following example demonstrates the configuration of the `Date` type, that is internally supported by default.


It would be better to use a different type than the default.

sindresorhus · 2020-04-23T07:03:37Z

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.

sbencoding · 2020-04-23T17:21:19Z

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.

sindresorhus · 2024-02-28T07:59:32Z

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

sindresorhus requested changes Sep 22, 2019

View reviewed changes

sbencoding added 12 commits March 15, 2020 12:03

Add ability to store and get Date objects

c5aec65

Made it easier to add extra types

48c23a9

Convert dates when in a nested object Move conversion to store getter, setter

Test for getting, setting date within nested object

6b89d13

Add typescript tests for Date objects Add typescript definition of extraTypes array

Update docs with automatic object serialization

1ffb1a7

Fix typescript test

70197bc

Made it easier to add extra types

98cc4c3

Convert dates when in a nested object Move conversion to store getter, setter

Test for getting, setting date within nested object

0ff7707

Add typescript tests for Date objects Add typescript definition of extraTypes array

Implement some requested changes

08b6c78

Fix typedef file

a777760

Fix wrong merging with upstream

d2f2906

Add circular handling, needs more testing

680ef66

Fix multi-level circular serialization + tests

1112ebd

sbencoding force-pushed the master branch from 9759f40 to 1112ebd Compare March 15, 2020 14:03

sindresorhus requested changes Apr 7, 2020

View reviewed changes

sindresorhus changed the title ~~Store and get Date objects~~ Add support for setting/getting Date objects Apr 7, 2020

sbencoding added 4 commits April 7, 2020 21:52

Fix typescript doc and styles

551daa2

Add tests for circular objects with normal types

29ea854

Make de/serialize methods cleaner and shorter

b32946d

Add documentation for adding extra types

c198a72

sbencoding added 2 commits April 8, 2020 15:02

Implement addition of extra types via the options argument in the con…

7e91c87

…structor

Add test for runtime addition of extra type

a95c60d

sindresorhus reviewed Apr 23, 2020

View reviewed changes

Update custom type example, fix and sync docs

7483b12

Base automatically changed from master to main January 23, 2021 08:51

sindresorhus mentioned this pull request Mar 22, 2021

Support non-primitive values sindresorhus/electron-store#171

Closed

sindresorhus closed this Feb 28, 2024

	if (objectSeenList.indexOf(current) === -1) {
	if (!objectSeenList.includes(current)) {

Add support for setting/getting Date objects #89

Add support for setting/getting Date objects #89

Conversation

sbencoding commented Sep 10, 2019

sbencoding commented Sep 10, 2019

sindresorhus commented Sep 10, 2019

sbencoding commented Sep 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

delewis13 commented Feb 10, 2020

sbencoding commented Feb 13, 2020

papb commented Feb 14, 2020

sbencoding commented Mar 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbencoding commented Apr 7, 2020

sbencoding commented Apr 8, 2020

Choose a reason for hiding this comment

sindresorhus commented Apr 23, 2020

sbencoding commented Apr 23, 2020

sindresorhus commented Feb 28, 2024