Skip to content
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

ES6 refactor #185

Merged
merged 14 commits into from Jul 19, 2019
Merged

ES6 refactor #185

merged 14 commits into from Jul 19, 2019

Conversation

jbienkowski311
Copy link
Contributor

@jbienkowski311 jbienkowski311 commented Jul 10, 2019

This pull request is a first step to move this project closer to the present times.

Description

  1. Refactor ZooKeeper and its promisified version to ES6 standard.
  2. Get rid of lib/promise, instead use native Promise class.
  3. Fix tests, so they pass.

Motivation and Context

This project remembers the old days of Node 0.x. A lot has changed since then and we should move on. This PR is a first step towards better code quality.

As there are a lot of changes I submit this PR as is for now. I will however start working on enforcing the eslint rules, so we should end up with a project that is nice and tidy.

TODO

  • Make eslint pass
  • Explicitly define API input parameters

How Has This Been Tested?

It has been tested with both unit and integration tests.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

@jbienkowski311 jbienkowski311 changed the title ES6 refactor [WIP] ES6 refactor Jul 10, 2019
@DavidVujic DavidVujic self-requested a review July 10, 2019 21:05
@DavidVujic
Copy link
Collaborator

Thank you for doing this long awaited task! It's a massive PR and I will need some time to review this.

@jbienkowski311
Copy link
Contributor Author

jbienkowski311 commented Jul 11, 2019

You can follow ESLint refactoring progress in a separate PR: jbienkowski311#1

@DavidVujic
Copy link
Collaborator

Awesome work @jbienkowski311!

@jbienkowski311
Copy link
Contributor Author

jbienkowski311 commented Jul 11, 2019

I've merged eslint branch and resolved the conflicts. I did not enable ESLint rules on tests_integration directory - it is a lot of work to make it pass and I do not think that it is worth the effort. After all, it's just a bunch of tests.

What I would like to do before announcing that this PR ready to merge is explicit definition of the input parameters for the API methods. Before, the library was using arguments, right now I use ...args as a temporary solution to make ESLint pass. However I believe in rule that explicit is better than implicit 😉

PS: You can consider #184 as done 😉

@jbienkowski311 jbienkowski311 changed the title [WIP] ES6 refactor ES6 refactor Jul 12, 2019
@jbienkowski311
Copy link
Contributor Author

Okay, I think that's enough for this PR. @DavidVujic please check if I did not mess up with the API (I used documentation from README.md and tests). I've added a_sync method to the documentation as it was missing. Not sure if it's correct though... 😉

@DavidVujic
Copy link
Collaborator

Thank you @jbienkowski311! I'll have a look. I would like to finish issue #153 and build a new release from it before merging this branch. Are you ok with that?

@jbienkowski311
Copy link
Contributor Author

Sure thing!

Copy link
Collaborator

@DavidVujic DavidVujic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! I have a question about the legacy Promise library that is used in the Master branch: this PR with native Promises will be a breaking change, won't it?

If possible, to avoid breaking changes - could we add wrappers in the ZK Promise class for the old Promise lib functions? They could be marked with JsDoc "deprecated" comments. What do you think about it?

package.json Show resolved Hide resolved
lib/zookeeper.js Show resolved Hide resolved
lib/zookeeper.js Show resolved Hide resolved
@jbienkowski311
Copy link
Contributor Author

Great work! I have a question about the legacy Promise library that is used in the Master branch: this PR with native Promises will be a breaking change, won't it?

If possible, to avoid breaking changes - could we add wrappers in the ZK Promise class for the old Promise lib functions? They could be marked with JsDoc "deprecated" comments. What do you think about it?

Now this could be a potential breaking change... However it is an easy fix to implement. Just give me few hours 😉

@jbienkowski311
Copy link
Contributor Author

@DavidVujic I brought back the ZkPromise class with previously missing methods being deprecated as suggested. There was one method that I was unable to restore - Promise.wait. The good news is that it would not work anyway 😉 I have covered new ZkPromise class with tests as well.

@DavidVujic
Copy link
Collaborator

Awesome! I'll try to finish the review in the next couple of days. Currently at a location with a poor connection 🐮 🐑 🐔 🇸🇪

@DavidVujic DavidVujic merged commit c456cd1 into yfinkelstein:master Jul 19, 2019
@jbienkowski311 jbienkowski311 deleted the es6 branch April 21, 2021 10:58
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.

None yet

2 participants