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

version 5 (rewrite) #84

Merged
merged 9 commits into from
Jul 25, 2021
Merged

version 5 (rewrite) #84

merged 9 commits into from
Jul 25, 2021

Conversation

mike-marcacci
Copy link
Owner

@mike-marcacci mike-marcacci commented Dec 12, 2020

This is a draft rewrite that modernizes the library. The goals are to:

Do note that this will be a major release as it contains breaking changes.

Most of these changes have been sitting on my laptop for some time and I have just been too busy to push this over the line. I'm creating a draft PR here so there's something to pick back up as I have a chance.

@gurupras
Copy link

Don't know if this is the right place to post this, feel free to delete it or tell me to move it elsewhere.

I've been enamored by an api akin to

redlock.acquire(resource, ttl, async () => {
  ...
})

where the resource is auto released at the end of the function---regardless of exceptions. If you're serious about going in the direction of auto-renewal, then I feel like we can absolutely get rid of the unlock API without too much difficulty.

@mike-marcacci
Copy link
Owner Author

@gurupras that is almost exactly what is being added in this PR 😜

  public async using<T>(
    resources: string[],
    duration: number,
    settings: Settings,
    routine?: (signal: RedlockAbortSignal) => T
  ): Promise<T>;

  public async using<T>(
    resources: string[],
    duration: number,
    routine: (signal: RedlockAbortSignal) => T
  ): Promise<T>;

One of these days I'll have a chance to finish it up!

@alexkvak
Copy link

rewrite tests using docker to create disposable redis instances/clusters

I guess the tests should be written for current version 4, not for 5. It helps to be sure that there are no regressions in next major version.

@mike-marcacci
Copy link
Owner Author

Hi folks! This major rewrite is finally looking the way I want. I still would like to add automated tests for compatibility with redis clusters, and more documentation would probably be helpful.

These changes essentially amount to a full rewrite, and include:

  • rewritten in TypeScript
  • dropped support for legacy APIs (callback, disposer)
  • introduction of new, more modern APIs (fixes Auto extend lock #73)
  • removal of bluebird promises
  • improved error handling (fixes More granular error codes #43)
  • stats collection for better observability
  • choice of ioredis as the only redis library for testing
  • publication as an ES module
  • use of GitHub actions for CI

I just published a pre-release on NPM as 5.0.0-alpha.0 and plan on merging this to main shortly, so that fixes or improvements can be more easily made against it, as it progresses towards a full release. Any patches to the current version will be made in the v4 branch once this is merged.

Any help testing this and vetting the changes would be appreciated!

@mike-marcacci mike-marcacci merged commit 4e5df01 into main Jul 25, 2021
@SimenB
Copy link

SimenB commented Feb 8, 2022

@mike-marcacci could you touch on why you don't wanna support redis anymore? It has moved into the official redis org on GH (https://github.com/redis/node-redis), so supporting it seems reasonable. If it's solely #68, it would be nice (if possible) to get a different fix than just "use ioredis" 😀

@you-fail-me
Copy link

you-fail-me commented Oct 6, 2023

+1 for @SimenB request. Apart from redis being close to "official client" there are valid cases where it would be preferred over ioredis, such as when using Elasticache where the latter is known for causing issues and being harder to use due to that.

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

5 participants