refactor: improve subscription api #3
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -36,8 +36,21 @@ export type History<T> = { | |
| * the history index of the current snapshot | ||
| */ | ||
| index: number; | ||
| /** | ||
| * a function to stop the internal subscription process | ||
| */ | ||
| unsubscribe: ReturnType<typeof subscribe>; | ||
| }; | ||
|
|
||
| type SubscribeOps = Parameters<Parameters<typeof subscribe>[1]>[0]; | ||
| type SubscribeCallback = (ops: SubscribeOps, historySaved: boolean) => void; | ||
| /** | ||
| * A field to either enable/disable the internal subscribe functionality. | ||
| * Optionally a callback function can be provided to hook into the | ||
| * internal subscribe handler. | ||
| */ | ||
| export type SkipSubscribeOrCallback = boolean | SubscribeCallback; | ||
|
|
||
| const isObject = (value: unknown): value is object => | ||
| !!value && typeof value === 'object'; | ||
|
|
||
|
|
@@ -64,9 +77,6 @@ const deepClone = <T>(value: T): T => { | |
| * It includes following main properties:<br> | ||
| * - value: any value (does not have to be an object)<br> | ||
| * - history: an object holding the history of snapshots and other metadata<br> | ||
| * - canUndo: a function to return true if undo is available <br> | ||
| * - undo: a function to go back history <br> | ||
| * - canRedo: a function to return true if redo is available <br> | ||
|
|
@@ -82,7 +92,8 @@ const deepClone = <T>(value: T): T => { | |
| * - Suspense/promise is not supported. <br> | ||
| * | ||
| * @param initialValue - any object to track | ||
| * @param skipSubscribeOrCallback - determines if the internal subscribe behaviour should be skipped. Optionally, | ||
| * a callback function can be provided. | ||
| * @returns proxyObject | ||
| * | ||
| * @example | ||
|
|
@@ -91,7 +102,10 @@ const deepClone = <T>(value: T): T => { | |
| * count: 1, | ||
| * }) | ||
| */ | ||
| export function proxyWithHistory<V>( | ||
| initialValue: V, | ||
| skipSubscribeOrCallback: SkipSubscribeOrCallback = false | ||
| ) { | ||
| const proxyObject = proxy({ | ||
| /** | ||
| * any value to be tracked (does not have to be an object) | ||
|
|
@@ -101,12 +115,14 @@ export function proxyWithHistory<V>(initialValue: V, skipSubscribe = false) { | |
| * an object holding the history of snapshots and other metadata <br> | ||
| * - history.index: the history index to the current snapshot <br> | ||
| * - history.nodes: the nodes of the history for each change <br> | ||
| * - history.wip: field for holding sandbox changes; used to avoid infinite loops <br> | ||
| * - history.unsubscribe: a function to stop the internal subscription process <br> | ||
| */ | ||
| history: ref<History<V>>({ | ||
| wip: undefined, // to avoid infinite loop | ||
| nodes: [], | ||
| index: -1, | ||
| unsubscribe: () => {}, | ||
|
There was a problem hiding this comment. I'm not sure if having unsubscribe here is a reasonable abstraction. I'd prefer paring There was a problem hiding this comment. i actually tried this but i would have to wrap unsubscribe with ref as well which didnt work out for me There was a problem hiding this comment. this is because it is assigned after subscribe and artificially changes the history nodes There was a problem hiding this comment. I would prefer either of these patterns: const unsubscribe = foo.subscribe(...);
const unsubscribe = subscribe(foo, ...);
subscribe(foo, ...); unsubscribe(foo, ...);in our case, the last one would be possible. but at the same time, it wouldn't look very nice. back up a little bit, what's the motivation of unsubscribe in this case, and what are the use cases? There was a problem hiding this comment. I mainly added the unsubscribe method as a way to do cleanup for eg. if a component in react unmounts an d the subscription is no longer needed. if this is not needed then i guess we can remove it for now. if it is needed then maybe we move both subscribe and unsubscribe into There was a problem hiding this comment. One solution I could think of without changing the code is:
There was a problem hiding this comment.
I think you can define There was a problem hiding this comment. K I will stash this change then and split the pr There was a problem hiding this comment.
Will go with this for now. in the next PR (#5 ), I however, extracted the utility to check if the history save should happen. this should reduce the amount of functionality to copy/rewrite for the consumer |
||
| }), | ||
| /** | ||
| * get the date when a node was entered into history. | ||
|
|
@@ -196,14 +212,15 @@ export function proxyWithHistory<V>(initialValue: V, skipSubscribe = false) { | |
| */ | ||
| subscribe: () => | ||
| subscribe(proxyObject, (ops) => { | ||
| const shouldSaveHistory = ops.every( | ||
| (op) => | ||
| op[1][0] === 'value' && | ||
| (op[0] !== 'set' || op[2] !== proxyObject.history.wip) | ||
| ); | ||
|
|
||
| if (shouldSaveHistory) proxyObject.saveHistory(); | ||
| if (typeof skipSubscribeOrCallback === 'function') { | ||
| skipSubscribeOrCallback(ops, shouldSaveHistory); | ||
| } | ||
| }), | ||
|
|
||
|
|
@@ -283,8 +300,8 @@ export function proxyWithHistory<V>(initialValue: V, skipSubscribe = false) { | |
|
|
||
| proxyObject.saveHistory(); | ||
|
|
||
| if (skipSubscribeOrCallback !== true) { | ||
| proxyObject.history.unsubscribe = proxyObject.subscribe(); | ||
| } | ||
|
|
||
| return proxyObject; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may consider making the second argument option object for future extension.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup true