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.

Sign up for GitHub

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

Jump to bottom

feat(Client): add global sweepers #6825

Merged
merged 12 commits into from Dec 15, 2021
Merged

feat(Client): add global sweepers #6825

merged 12 commits into from Dec 15, 2021

Conversation

ckohen
Copy link
Member

@ckohen ckohen commented Oct 13, 2021 •
edited

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

⚠️ This is tested, but not all combinations of sweepers have been validated.
  • This aims to tackle point 2 in Roadmap: cache and sweeping improvements #6539: Global Sweepers. This does not completely satisfy what was outlined there however, it still relies on LimitedCollection to provide max size functionality. After doing the work to get to this point, that seemed out of scope, and can easily be tackled in another PR. This does mean that you are still storing extra properties on LimitedCollection for now, however hopefully not tracking a timeout.

In order to keep this semver: minor, default options have unfortunately not been switched to use the sweepers in this PR.

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)

vladfrangu
vladfrangu previously requested changes Oct 13, 2021
View reviewed changes
Copy link
Member

@vladfrangu vladfrangu left a comment

Choose a reason for hiding this comment

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

At first glance..this looks awesome!

src/client/Client.js Outdated Show resolved Hide resolved
src/util/Sweepers.js Outdated Show resolved Hide resolved
typings/index.d.ts Outdated Show resolved Hide resolved
ImRodry
ImRodry reviewed Oct 13, 2021
View reviewed changes
src/client/Client.js Outdated Show resolved Hide resolved
almeidx
almeidx requested changes Oct 13, 2021
View reviewed changes
src/util/Sweepers.js Outdated Show resolved Hide resolved
@iCrawl iCrawl added this to the Version 13.3 milestone Oct 14, 2021
vladfrangu
vladfrangu reviewed Oct 14, 2021
View reviewed changes
src/util/Sweepers.js Outdated Show resolved Hide resolved
@iCrawl iCrawl modified the milestones: Version 13.3, Version 13.4 Oct 28, 2021
@iCrawl iCrawl modified the milestones: Version 13.4, Version 13.x Nov 11, 2021
@kyranet kyranet mentioned this pull request Nov 24, 2021
Merged
@ckohen ckohen force-pushed the global-sweepers branch 2 times, most recently from 4a7650e to 4152aa0 Compare December 1, 2021 12:56
kyranet
kyranet requested changes Dec 8, 2021
View reviewed changes
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.

Just a few things, nice work overall ^^

src/util/Sweepers.js Outdated Show resolved Hide resolved
src/util/Sweepers.js Show resolved Hide resolved
src/util/Sweepers.js Outdated Show resolved Hide resolved
ckohen and others added 7 commits December 9, 2021 02:24
@ckohen @kyranet
feat(Client): add global sweepers
154965e 
Co-authored-by: Antonio Román <kyradiscord@gmail.com>
@ckohen
types(Sweepers): add types for global sweepers
eaf294d
@ckohen @vladfrangu
refactor(Sweepers): cache the cache
e91e607 
Co-Authored-By: Vlad Frangu <kingdgrizzle@gmail.com>
@ckohen @ImRodry @almeidx
refactor: address pr comments
8c98558 
Co-Authored-By: Rodry <38259440+ImRodry@users.noreply.github.com>
Co-Authored-By: Almeida <almeidx@pm.me>
@ckohen
refactor(Sweepers): make intervals a record
7b046fa
@ckohen
fix: testing
a059e9a
@ckohen
feat(Sweepers): add util functions and default settings
895a6dc
ckohen and others added 2 commits December 9, 2021 02:24
@ckohen @kyranet
chore: better indentation, less mutation
4c8d894 
Co-Authored-By: Antonio Román <kyradiscord@gmail.com>
@ckohen
docs: finalize deprecations
9a2594a
kyranet
kyranet previously requested changes Dec 9, 2021
View reviewed changes
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.

Just the last 2 requests, everything else looks fine to me.

src/util/Sweepers.js Show resolved Hide resolved
src/util/Sweepers.js Outdated Show resolved Hide resolved
@kyranet kyranet mentioned this pull request Dec 9, 2021
Closed
@ckohen @kyranet
refactor: use existing array
c5d9d27 
Co-authored-by: Antonio Román <kyradiscord@gmail.com>
@iCrawl iCrawl requested a review from kyranet December 10, 2021 09:11
vladfrangu
vladfrangu requested changes Dec 11, 2021
View reviewed changes
src/util/Options.js Show resolved Hide resolved
@iCrawl iCrawl modified the milestones: Version 13.x, Version 13.4 Dec 13, 2021
SpaceEEC
SpaceEEC requested changes Dec 14, 2021
View reviewed changes
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.

I'm not sure about the importance of the count bits, but here they are. 😅

src/util/Sweepers.js Outdated Show resolved Hide resolved
src/util/Sweepers.js Outdated Show resolved Hide resolved
src/util/Sweepers.js Outdated Show resolved Hide resolved
src/util/Sweepers.js Outdated Show resolved Hide resolved
src/util/Sweepers.js Outdated Show resolved Hide resolved
@ckohen @kyranet
refactor: let sweep handle empty caches
8a44e87 
Co-Authored-By: Antonio Román <kyradiscord@gmail.com>
@iCrawl iCrawl merged commit d1ef2f5 into discordjs:main Dec 15, 2021
@ckohen ckohen deleted the global-sweepers branch December 16, 2021 09:26
@ckohen ckohen mentioned this pull request Dec 16, 2021
Merged
@attituding attituding mentioned this pull request Dec 30, 2021
Merged
@kyranet kyranet mentioned this pull request Feb 16, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kyranet kyranet kyranet approved these changes

@Jiralite Jiralite Jiralite left review comments

@ImRodry ImRodry ImRodry left review comments

@almeidx almeidx almeidx requested changes

@vladfrangu vladfrangu vladfrangu approved these changes

@iCrawl iCrawl iCrawl approved these changes

@SpaceEEC SpaceEEC SpaceEEC approved these changes

Assignees
No one assigned
Labels
caching semver:minor
Projects
discord.js
Archived in project
Milestone
Version 13.4
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@ckohen @vladfrangu @iCrawl @kyranet @SpaceEEC @Jiralite @ImRodry @almeidx