Skip to content

Conversation

beaucollins
Copy link
Contributor

@beaucollins beaucollins commented Feb 7, 2018

The Channel object is too nosy and overrides instance methods on the Bucket which leads to indirect code paths.

These changes remove the knowledge of a Bucket instance from the Channel. It is now up to the Bucket to use the correct Channel APIs and listen for the correct Channel events.

From a 3rd party's perspective the api has not changed.

Bucket Promise API

The Bucket interface now provides a Promise based interface. All methods that take a callback also return a Promise:

// Still works
bucket.get( 'my-object', callback );

// Now you can
bucket.get( 'my-object' ).then( success, failure );

// And async/await if you want to get rid of all the indents
const object = await bucket.get( 'my-object' );

Additional Cleanup

  • Stops listening to its own events. Event emit/listener pairs aren't necessary for an object that both generates and listens for those events.
  • Now that a bucket calls methods directly on a Channel we can get rid of bucketEvents
  • Bucket/Channel API is more clearly defined through .prototype. methods on Channel
  • JSDoc added to methods called by the Bucket (and some additional ones as well).
  • Snuck in the node-uuid to uuid dependency update

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.
Copy link
Contributor

@roundhill roundhill left a 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.

callbackAsPromise( store.update.bind( store, id, object, isIndexing ) ),
remove: ( id ) =>
callbackAsPromise( store.remove.bind( store, id ) ),
find: ( query ) =>
Copy link
Contributor

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 =>

reject( error );
return;
}
resolve( result );
Copy link
Contributor

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 );

@beaucollins
Copy link
Contributor Author

This should be backwards compatible.

@roundhill
Copy link
Contributor

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.

@beaucollins
Copy link
Contributor Author

There's two pieces to consider for backwards compatibility:

  • The Bucket API keeping callbacks along with promises
  • The BucketStore API that uses callbacks that this PR now changes the API for internally to a Promise based one.

Simplenote currently uses both of these.

I think a separate PR to make those breaking changes makes sense.

@beaucollins
Copy link
Contributor Author

@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.

@roundhill
Copy link
Contributor

@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.

@dmsnell
Copy link
Contributor

dmsnell commented Feb 19, 2018

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.

@beaucollins beaucollins merged commit 54a44c4 into master Feb 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants