-
Notifications
You must be signed in to change notification settings - Fork 12
Channel/Bucket interface refactor #56
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
A bucket instance is given a channel instance and it uses the appropriate methods to communicate with the channel
Lets external API's remain backwards compatible
Test still depends the `change-version' event so emitting it at the correct time.
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.
I like the changes. I wonder if we need to make it backwards compatible? Not sure if this is being used in anything besides Simplenote... but maybe better safe than sorry.
src/simperium/bucket.js
Outdated
callbackAsPromise( store.update.bind( store, id, object, isIndexing ) ), | ||
remove: ( id ) => | ||
callbackAsPromise( store.remove.bind( store, id ) ), | ||
find: ( query ) => |
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.
How do you feel about removing the parens for one parameter? find: query =>
src/simperium/bucket.js
Outdated
reject( error ); | ||
return; | ||
} | ||
resolve( result ); |
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.
What if you one lined this block:
error ? reject( error ) : resolve( result );
This should be backwards compatible. |
Sorry, I meant maybe we don't need to add the compatibility code? Move forward, blaze a new trail and all that. |
- implements suggested fixes by @danroundhill
There's two pieces to consider for backwards compatibility:
Simplenote currently uses both of these. I think a separate PR to make those breaking changes makes sense. |
@roundhill I just gave this a quick run through with Simplenote and it appeared to be working. I didn't exercise a lot of the app. |
ok, I will give it a test in Simplenote as well. |
wow I wish I had looked here before spending way too long trying to understand the circular references in the channel and bucket messaging! @roundhill there's always old versions of the npm package that aren't going away if people can't update yet. we can major-bump-it. |
The
Channel
object is too nosy and overrides instance methods on theBucket
which leads to indirect code paths.These changes remove the knowledge of a
Bucket
instance from theChannel
. It is now up to theBucket
to use the correctChannel
APIs and listen for the correctChannel
events.From a 3rd party's perspective the api has not changed.
Bucket Promise API
The
Bucket
interface now provides aPromise
based interface. All methods that take a callback also return aPromise
:Additional Cleanup
Channel
we can get rid ofbucketEvents
.prototype.
methods onChannel
Bucket
(and some additional ones as well).node-uuid
touuid
dependency update