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

feat(Managers): add customizable caching for managers #6013

Merged
merged 14 commits into from
Jul 3, 2021

Conversation

1Computer1
Copy link
Contributor

@1Computer1 1Computer1 commented Jul 3, 2021

Please describe the changes this PR makes and why it should be merged:

  • All managers are actually managers now:

    • Hierarchy is BaseManager :> DataManager :> CachedManager.
    • BaseManager has no cache, DataManager has a cache getter and the resolve methods, CachedManager is the whole thing.
  • Customizable cache types for CachedManagers:

    • Anything implementing the Collection interface can be used.
    • You can disable caching now, you're welcome.
    • Example:
// Using a helper method that does reasonable stuff.
const client = new Client({
  makeCache: Options.cacheWithLimits({
    MessageManager: 200, // This is default.
    PresenceManager: 0, // Add more class names here.
  }),
});

// Just do whatever.
const client = new Client({
  makeCache: (manager) => {
    if (manager.name === 'MessageManager') return new LimitedCollection(0);
    return new Coolection();
  }
});
  • Breaking changes in summary:

    • The new managers' hierarchy and methods and constructors.
    • ReactionUserManager's constructor, cus it was weird.
    • Moved options related stuff out of Constants into Options.
    • Removed messageCacheMaxSize.
  • Things I wanna do but too much work/infeasible with current designs:

    • Caches with async methods. Do this in the rewrite.
    • More cache types, e.g. one that sweeps based on a predicate (replacing the message sweeping). Need more design work.
    • Rename the manager add to something better, how about addToCache? Someone do this please.

Status and versioning classification:

  • Code changes have been tested against the Discord API, or there are no code changes
  • I know how to update typings and have done so, or typings don't need updating
  • This PR changes the library's interface (methods or parameters added)
  • This PR includes breaking changes (methods removed or renamed, parameters moved or removed)

@1Computer1 1Computer1 marked this pull request as ready for review July 3, 2021 04:23
src/util/Options.js Outdated Show resolved Hide resolved
src/util/Options.js Outdated Show resolved Hide resolved
src/managers/DataManager.js Outdated Show resolved Hide resolved
src/managers/DataManager.js Show resolved Hide resolved
src/managers/DataManager.js Show resolved Hide resolved
src/managers/DataManager.js Outdated Show resolved Hide resolved
src/managers/DataManager.js Outdated Show resolved Hide resolved
test/tester2000.js Outdated Show resolved Hide resolved
kyranet
kyranet previously requested changes Jul 3, 2021
src/managers/ReactionUserManager.js Outdated Show resolved Hide resolved
src/managers/VoiceStateManager.js Outdated Show resolved Hide resolved
src/client/Client.js Show resolved Hide resolved
@iCrawl iCrawl added this to the Version 13 milestone Jul 3, 2021
src/util/Options.js Outdated Show resolved Hide resolved
src/managers/CachedManager.js Show resolved Hide resolved
src/util/Options.js Show resolved Hide resolved
src/util/Options.js Outdated Show resolved Hide resolved
typings/index.d.ts Outdated Show resolved Hide resolved
@iCrawl
Copy link
Member

iCrawl commented Jul 3, 2021

This needs a rebase.

@iCrawl iCrawl requested a review from kyranet July 3, 2021 12:29
@iCrawl iCrawl linked an issue Jul 3, 2021 that may be closed by this pull request
@iCrawl iCrawl linked an issue Jul 3, 2021 that may be closed by this pull request
@iCrawl
Copy link
Member

iCrawl commented Jul 3, 2021

This needs a rebase.

@iCrawl iCrawl dismissed stale reviews from vladfrangu and SpaceEEC July 3, 2021 20:02

Stale

Copy link
Member

@SpaceEEC SpaceEEC left a comment

Choose a reason for hiding this comment

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

It might be worth a consideration to make LimitedCollection public again, otherwise you'd have to reimplement it yourself if cacheWithLimits doesn't cut it for somereason.

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.

Ability to disable caching users & members cacheType change or implementation.
6 participants