main #8

kote-isaev commented 2023年09月16日 08:42:48 +02:00

Changes:

1. Fixed how database path built from url, in more cross-platform way, to fix issue experienced under local development under Windows
2. made log muting more testable and get console log muting tests working.

Changes: 1) Fixed how database path built from url, in more cross-platform way, to fix issue experienced under local development under Windows 2) made log muting more testable and get console log muting tests working.

kote-isaev commented 2023年09月17日 07:02:08 +02:00

Added fix for assigning property to null and undefined.
Null work as expexted (property assigned to null and saved), assign to undefined works as delete property.

Added fix for assigning property to null and undefined. Null work as expexted (property assigned to null and saved), assign to undefined works as delete property.

aral commented 2023年09月18日 15:09:40 +02:00

@kote-isaev Thanks again for this, Kote.

I’ve cherry-picked the Windows fix (I’d missed it in the tests – it’s an issue I’d previously raised with the Node folks @ https://github.com/nodejs/node/issues/37845 – thank you for catching this) and the null/undefined implementations.

For undefined, I went ahead and made it so that instead of deleting the record, it actually stores undefined. This is so that it behaves the same regardless of whether you’re setting undefined in an object when creating it or later.

So:

db.myTable.myObject = { a: undefined }

And

db.myTable.myObject = {}
db.myTable.myObject.a = undefined

Now produce the same result;

{ a: undefined }

Other than that, I wasn’t sure what the console.log changes improved regarding the unit tests so I’ve left them out for now. If there is a limitation I’m missing with those tests that your code improves, would you mind detailing it in an issue and I’ll reconsider.

Thanks again for contributing and helping make JSDB better; I really appreciate it :)

💕

@kote-isaev Thanks again for this, Kote. I’ve cherry-picked the Windows fix (I’d missed it in the tests – it’s an issue I’d previously raised with the Node folks @ https://github.com/nodejs/node/issues/37845 – thank you for catching this) and the null/undefined implementations. For undefined, I went ahead and made it so that instead of deleting the record, it actually stores `undefined`. This is so that it behaves the same regardless of whether you’re setting `undefined` in an object when creating it or later. So: ```js db.myTable.myObject = { a: undefined } ``` And ```js db.myTable.myObject = {} db.myTable.myObject.a = undefined ``` Now produce the same result; ```js { a: undefined } ``` Other than that, I wasn’t sure what the `console.log` changes improved regarding the unit tests so I’ve left them out for now. If there is a limitation I’m missing with those tests that your code improves, would you mind detailing it in an issue and I’ll reconsider. Thanks again for contributing and helping make JSDB better; I really appreciate it :) 💕

aral commented 2023年09月18日 15:11:48 +02:00

The contents of this pull request have now been published in JSDB version 3.0.4.

Thanks again, @kote-isaev :)

The contents of this pull request have now been published in JSDB version 3.0.4. Thanks again, @kote-isaev :)

kote-isaev commented 2023年09月18日 16:12:07 +02:00

Thanks for quick response and accepting my pull request.
Lets see how this update about undefined will settle. From JSDB point of view it is correct (to keep behavior of assignment consistent).
My motivation for deleting property instead was the fact that undefined values are being skipped during JSON serialization that happens e. g. during sending a JSDB record as part of response body, or if that record will be JSON-ed as part of "JSON cloning" of record.
For that it is better to use structuralClone if it available, but it seems I was a bit over-engineering this aspect, may be.

Thanks for quick response and accepting my pull request. Lets see how this update about `undefined` will settle. From JSDB point of view it is correct (to keep behavior of assignment consistent). My motivation for deleting property instead was the fact that `undefined` values are being skipped during JSON serialization that happens e. g. during sending a JSDB record as part of response body, or if that record will be JSON-ed as part of "JSON cloning" of record. For that it is better to use `structuralClone` if it available, but it seems I was a bit over-engineering this aspect, may be.