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

Custom connectors and additional typings #906

Merged
merged 20 commits into from
Jun 25, 2019
Merged

Conversation

imatlopez
Copy link
Contributor

@imatlopez imatlopez commented Jun 23, 2019

Made the SentinelIterator and actual Iterator and made it not affect this.options.sentinels in place (tests were there to test the side effect).

Added floatingSentinels to the SentinelConnectorOptions which is called whenever connect is called, it must return an array of sentinels using a callback.

Welcome any feedback :)

resolves #903 for Sentinel at least :)

@luin
Copy link
Collaborator

luin commented Jun 24, 2019

Thanks for the pull request!

It adds some overheads to add an option for this specified issue. A better way seems to be customizing SentinelConnector by overriding SentinelConnector#connect() to fetch dynamic endpoints first.

Currently, it could be a little bit hacky since there's no way for users to provide customized connectors to Redis. But it trivial to add an option to Redis like connector which will solve a broader set of problems.

@imatlopez
Copy link
Contributor Author

imatlopez commented Jun 24, 2019

I added the connector property to the config, which is now where Redis uses its connector from. Hence if no property is set, the constructor will assign it based on the old behavior.

I reverted some of the changes to SentinelConnector but not all because I felt it is cleaner this way (?)

Added AsyncSentinelConnector as an example of a custom connector that can be passed in.

Modified index.ts to export some types (and make the module interoperable with es6 modules), so once typings are satisfactory, it's just a mater of exporting the types.

@imatlopez
Copy link
Contributor Author

@luin let me know your thoughts :)

lib/connectors/SentinelConnector/index.ts Outdated Show resolved Hide resolved
lib/connectors/SentinelConnector/index.ts Outdated Show resolved Hide resolved
lib/connectors/AsyncSentinelConnector.ts Outdated Show resolved Hide resolved
lib/connectors/AsyncSentinelConnector.ts Outdated Show resolved Hide resolved
@luin
Copy link
Collaborator

luin commented Jun 24, 2019

Awesome work! As you said, that changes made on SentinelConnector make the code cleaner. The most parts of codes are great, and I've left some comments about the interface of SentinelConnector since this the first time we export it.

});
}

}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest moving this file to the examples folder since most users don't need it, and it trivial to implement one. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 now I just need to find why the tests fail

@@ -12,6 +12,7 @@ services:
- redis-server

script:
- npm run build
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why npm run build is needed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

test may have type errors like trying to access private properties, but the code itself is fine, so this guards against build time errors while the tests are for logical errors only.

@imatlopez imatlopez changed the title Asynchronous sentinel fetching for floating ip services Custom connectors and additional typings Jun 24, 2019
@imatlopez
Copy link
Contributor Author

imatlopez commented Jun 24, 2019

I think I dug myself a deep hole trying to change the interface to promises. I haven't figured out how to run the tests locally either so I'm gonna need some assistance 😁

edit: scratch that... just went too far with the cleanup

@imatlopez
Copy link
Contributor Author

@luin I've noticed that the ESLint doesn't quite run in some parts of the code, fixing it by adding typescript-eslint added lots of conflicts. Once this goes in, would you like another PR adding it for code consistency? I personally like Prettier for code styling too.

@luin luin merged commit bf3fe29 into redis:master Jun 25, 2019
@luin
Copy link
Collaborator

luin commented Jun 25, 2019

@imatlopez That would be great! And Prettier is also my favorite :-)

ioredis-robot pushed a commit that referenced this pull request Jun 25, 2019
# [4.11.0](v4.10.4...v4.11.0) (2019-06-25)

### Features

* support custom connectors ([#906](#906)) ([bf3fe29](bf3fe29))
@ioredis-robot
Copy link
Collaborator

🎉 This PR is included in version 4.11.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

janus-dev87 added a commit to janus-dev87/ioredis-work that referenced this pull request Mar 1, 2024
# [4.11.0](redis/ioredis@v4.10.4...v4.11.0) (2019-06-25)

### Features

* support custom connectors ([#906](redis/ioredis#906)) ([bf3fe29](redis/ioredis@bf3fe29))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Async Sentinel/Cluster Addresses
3 participants