feathersjs-ecosystem / feathers-sync

Synchronize service events between Feathers application instances
MIT License
221 stars 41 forks source link

Custom serializer / deserializer support #130

Closed deskoh closed 4 years ago

deskoh commented 4 years ago

Summary

Related issue #128 #87.

daffl commented 4 years ago

Awesome! This will be really useful. Two small things:

1) Maybe we can add deserialize and serialize to app.sync that is set in each adapter (e.g. https://github.com/feathersjs-ecosystem/feathers-sync/blob/master/lib/adapters/amqp.js#L37). Then it needs to be called only once in https://github.com/feathersjs-ecosystem/feathers-sync/blob/master/lib/core.js#L16 and https://github.com/feathersjs-ecosystem/feathers-sync/blob/master/lib/core.js#L46 so it's always consistent between all adapters. 2) As a test I would just configure the Redis adapter to use the BSON serializer from the documentation so setting serializers is covered.

deskoh commented 4 years ago

Do you mean something like the following in https://github.com/feathersjs-ecosystem/feathers-sync/blob/master/lib/core.js?

app.on('sync-in', (rawData) => {
    const { event, path, data, context } = app.sync.deserialize(rawData);
    // ...
})

//...

return app.emit('sync-out', app.sync.serialize({
    event, path, data, context
}));

This means core.test.js would need a re-write as the default serializer / deserializer is defined when creating the adapter.

daffl commented 4 years ago

Yes, the core tests would probably need something like

app.sync = {
  serialize: data => data,
  deserialize: data => data
}

In https://github.com/feathersjs-ecosystem/feathers-sync/blob/master/test/core.test.js#L16

deskoh commented 4 years ago

Added BSON serialization tests for date values for redis and AMQP. Note that deepStrictEqual assertion will fail.

deskoh commented 4 years ago

@daffl any comments on this PR?

daffl commented 4 years ago

This is great, thank you! Released as v1.2.0

kc-dot-io commented 4 years ago

Excited for 1.2.0!