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(Util): add SweptCollection for auto sweeping of caches #6110

Merged
merged 23 commits into from
Jul 30, 2021

Conversation

ckohen
Copy link
Member

@ckohen ckohen commented Jul 13, 2021

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

⚠️ Updates Collection dep, including breaking changers
⚠️ Updates node requirement to 14.6.0 for Finalizers and WeakRef

Eslint config has been updated to target es2021 as well.

Adds SweptCollection (mainly for use with threads but has other uses). This can replace Client#sweepMessages, however with the loss of debug events, which is why it was not removed.

Other breaking change is the change in ThreadManager, to automatically sweep archived threads older than 4 hours every hour (default sweep settings)

I'm open to changes for these defaults, kinda just picked them at random.

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)

src/util/Options.js Outdated Show resolved Hide resolved
src/util/Options.js Outdated Show resolved Hide resolved
src/util/SweptCollection.js Outdated Show resolved Hide resolved
src/util/SweptCollection.js Outdated Show resolved Hide resolved
Copy link
Member

@kyranet kyranet left a comment

Choose a reason for hiding this comment

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

Also, just one tiny question... we need to access to the Client instance, in an object that is used to build the instance itself, do you have an example on how to use this?

src/util/SweptCollection.js Show resolved Hide resolved
@1Computer1
Copy link
Contributor

1Computer1 commented Jul 13, 2021

From the discord, my suggestions on the interface:

export class SweptCollection<K, V> extends Collection<K, V> {
  public constructor(options: SweptCollectionOptions<K, V>, iterable?: Iterable<readonly [K, V]>);
  public maxSize: number;
  public shouldKeep: ((value: V) => boolean) | null;
  public sweepInterval: number | null;
  public sweepFilter: (() => (value: V) => boolean) | null;
}

export interface SweptCollectionOptions<K, V> {
  maxSize?: number;
  shouldKeep?: (value: V) => boolean;
  sweepInterval?: number;
  sweepFilter?: () => (value: V) => boolean;
}

Usage example:

new SweptCollection({
  sweepFilter: () => {
    const now = Date.now();
    return (t) => {
      if (!t.createdTimestamp) return false;
      if (!t.archived) return false;
      return now - t.createdTimestamp > lifetimeMs;
    };
  }
});

// Can make some utilities for users:
new SweptCollection({
  sweepFilter: SweptCollection.filterByLifetime({
    maxLifetime: lifetimeMs,
    getCreationTime: t => t.createdTimestamp,
    shouldKeep: t => !t.archived
  })
});
  • Possibly remove letting users keep items at max cache due to the fact that set is now O(n).

  • Too many cacheWiths now, I think it's best to do this perhaps:

public static cacheByManager(fs: Record<string, () => Collection<any, any>>): CacheFactory { ... }

@ckohen
Copy link
Member Author

ckohen commented Jul 14, 2021

Did most of what comp suggested, couple of name changes to make it clearer what these things do.

The only thing I didn't touch:

  • Too many cacheWiths now, I think it's best to do this perhaps:
public static cacheByManager(fs: Record<string, () => Collection<any, any>>): CacheFactory { ... }

What's the point in this, this seems harder than just implementing your own makeCache function. In fact, the only difference is that you're not building the default case.

src/util/Options.js Outdated Show resolved Hide resolved
src/util/SweptCollection.js Outdated Show resolved Hide resolved
src/util/SweptCollection.js Outdated Show resolved Hide resolved
src/util/SweptCollection.js Outdated Show resolved Hide resolved
typings/index.d.ts Outdated Show resolved Hide resolved
typings/index.d.ts Outdated Show resolved Hide resolved
typings/index.d.ts Outdated Show resolved Hide resolved
src/util/SweptCollection.js Outdated Show resolved Hide resolved
src/util/SweptCollection.js Show resolved Hide resolved
src/util/Options.js Outdated Show resolved Hide resolved
src/util/Options.js Outdated Show resolved Hide resolved
src/util/Options.js Outdated Show resolved Hide resolved
src/util/SweptCollection.js Outdated Show resolved Hide resolved
src/util/SweptCollection.js Show resolved Hide resolved
src/util/SweptCollection.js Outdated Show resolved Hide resolved
src/util/SweptCollection.js Outdated Show resolved Hide resolved
src/util/SweptCollection.js Outdated Show resolved Hide resolved
src/util/SweptCollection.js Outdated Show resolved Hide resolved
@iCrawl iCrawl added this to the Version 13 milestone Jul 15, 2021
@SpaceEEC SpaceEEC linked an issue Jul 16, 2021 that may be closed by this pull request
src/util/SweptCollection.js Show resolved Hide resolved
typings/index.d.ts Outdated Show resolved Hide resolved
typings/index.d.ts Outdated Show resolved Hide resolved
typings/index.d.ts Outdated Show resolved Hide resolved
src/util/Options.js Outdated Show resolved Hide resolved
src/managers/GuildEmojiRoleManager.js Outdated Show resolved Hide resolved
src/util/SweptCollection.js Show resolved Hide resolved
ckohen and others added 7 commits July 27, 2021 19:44
src/managers/CachedManager.js Outdated Show resolved Hide resolved
src/managers/GuildEmojiRoleManager.js Outdated Show resolved Hide resolved
src/managers/GuildEmojiRoleManager.js Outdated Show resolved Hide resolved
src/managers/GuildMemberRoleManager.js Outdated Show resolved Hide resolved
src/util/Options.js Outdated Show resolved Hide resolved
@iCrawl iCrawl merged commit dbb59ba into discordjs:master Jul 30, 2021
@ckohen ckohen deleted the swept-collection branch July 31, 2021 00:45
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.

Message Sweep not for manually fetched messages
9 participants