configurable keyPath for people having existing records

[ghost]

Hi,

Please read the edit below.

There is a bug which prevents deletion of records from the store: createTombstone gets called with the user defined record: https://github.com/paldepind/synceddb/blob/master/client/src/utils.js#L116

But it has a hard coded scheme, which expects properties like key and version: https://github.com/paldepind/synceddb/blob/master/client/src/utils.js#L162

So in my stores, my primary key is id and not key, so when the tombstone gets put into the store, a new record is created (with a new id), which prevents the deletion from being noticed.

Possible solution: Pass the primary key field name and its value to createTombstone.

Thank you for your great work!

Edit: I just noticed that a property with name key seems to be generated automatically for newly created records. I'm using a db with existing entities, so it doesn't have that.

[piglovesyou]

It is the expected behaviour for now, SyncedDB only supports key as a primary key. If we support arbitrary name for it, we have to identify the name for client and server, then we need a config file or something for that, which makes us confused. See also here.

But I agree that this can cause a migration problem for person like you who wants to use existing data.

[ghost]

Ah, I see. But isn't this rather unflexible?

Would you accept a PR which will still defaults to key but lets the user specify the key path, if desired? On the other hand, key seems to be a generated "unique" value, which probably has some other function I don't get yet... If key is essential for synceddb, maybe it could be a unique index instead, letting the user choose the key path. Just a few thoughts.

I'll read more into your code and IndexedDB the next few days before attempting any changes. :)

[piglovesyou]

Actually it's @paldepind 's code 😃 Yeah let's do that, it should encourage adoption for such people. I think all we have to do is to make it able to pass keyPath option to both the client and the backend, edit hardcoded parts, then it should work.

[ghost]

I'll close this, since synceddb doesn't play well with my personal preferences for syncing. Some personal reasons/things that I noticed:

• key is actually a generated unique id; it can't be just an auto incrementing number, because it could collide on the server side (if e. g. both a client A and a client B create different offline versions of an object with key 1).
• Small bug: The MySQL persistence code creates a column key as INT, so the generated key doesn't get stored properly (using varchar should fix the problem).
• So if I'd want a numeric, maybe even auto incrementing PK that is not identical to key, I'd have to heavily change the source code.
• Making the property name key configurable would really mess up the source, because so many functions in utils.js use the key-property directly. A utils-Factory or sth. like that would be necessary in order the change the name.
• Suggestion: Use property-prefixes like _sdb_ to indicate that a property doesn't belong to the "real" record.
• Suggestion: Update the readme so ppl know they need to have a PK named key which must be computed by synceddb and can't be numerical.

Thanks for your support and respect for that sophisticated source code!

[piglovesyou]

@whatda That list definitely helps us to think core arch, thanks. As you said offline app should care uniqueness of id in each records (any clustering DB env can also say the same thing in the first place) so auto increment is never be an option.

I still feel configurable keyPath is good for some people so let me keep open it. Also let me change the summary