refactor: improve subscription api

[lwhiteley]

• add goTo utility
• improve internal subscription api
  • allow to provide callback for simpler hooking
  • expose history.unsubscribe handler for cleanup
  • allow second param of proxyWithHistory to be optionally a callback

[dai-shi]

looks good overall. added some minor comments.

[dai-shi]

You may consider making the second argument option object for future extension.

[lwhiteley]

yup true

[dai-shi]

I'm not sure if having unsubscribe here is a reasonable abstraction. I'd prefer paring subscribe and unsubscribe. I don't have a good idea, because it subscribe behind the scene, but if we have unsubscribe here, we would feel like supporting more edge cases like subscribing twice, unsubscribing twice and so on.

[lwhiteley]

i actually tried this but i would have to wrap unsubscribe with ref as well which didnt work out for me

[lwhiteley]

this is because it is assigned after subscribe and artificially changes the history nodes

[dai-shi]

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?

[lwhiteley]

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 history

[dai-shi]

One solution I could think of without changing the code is:

• note clarify that the default behavior is not able to clean up.
• if one needs to unsubscribe, they should use skipSubscribe=true and manually subscribe().

[dai-shi]

i actually tried this but i would have to wrap unsubscribe with ref as well which didnt work out for me

I think you can define unsubscribe method, and internally call a function in the history ref. That seems acceptable.

[lwhiteley]

K I will stash this change then and split the pr

[lwhiteley]

if one needs to unsubscribe, they should use skipSubscribe=true and manually subscribe().

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

[lwhiteley]

closing for now