[RFC] SGv2.1.1 #256
[RFC] SGv2.1.1 #256
Conversation
kyranet
added
Type: Enhancement
src/lib/settings/GatewayDriver.js
Outdated
| register(name, schema = {}, options = {}) { | ||
| register(name, defaultSchema = {}, { download = true, provider = this.client.options.providers.default } = {}) { | ||
| if (typeof name !== 'string') throw new TypeError('You must pass a name for your new gateway and it must be a string.'); | ||
| if (!this.client.methods.util.isObject(defaultSchema)) throw new TypeError('Schema must be a valid object or left undefined for an empty object.'); |
There was a problem hiding this comment.
Should just require Util here
src/lib/Client.js
Outdated
| this.gateways.register('users', undefined, this.options.gateways.users, false); | ||
| this.gateways.register('clientStorage', this.gateways.clientStorageSchema, this.options.gateways.clientStorage, false); | ||
| this.gateways.register('guilds', this.gateways.guildsSchema, { download: false, ...this.options.gateways.guilds }); | ||
| this.gateways.register('users', {}, { download: false, ...this.options.gateways.users }); |
There was a problem hiding this comment.
Should add a gateways.userSchema option and set it here instead of an empty object, even if the option by default is an empty object, keeps it consistent with the other ones
| } | ||
| throw new TypeError(`Invalid value. Expected object, string or Array<string>. Got: ${getDeepTypeName(key)}`); | ||
| if (typeof options === 'undefined' && guild && guild.constructor.name === 'Object') [guild, options] = [null, guild]; | ||
| if (guild) guild = this.gateway._resolveGuild(guild); |
There was a problem hiding this comment.
This should just be changed to use the internal resolver for Klasa. There's no reason for this method to be duplicated for Settings when it should be relatively the same.
There was a problem hiding this comment.
The problem with this is that at the moment, the Guild's SettingResolver is incredibly slow and requires await (Configuration#update doesn't use async for performance). Plus it takes three arguments and the error message is misleading.
src/lib/settings/Gateway.js
Outdated
| @@ -176,19 +107,8 @@ class Gateway extends GatewayStorage { | |||
| */ | |||
| async sync(input, download) { | |||
| if (typeof input === 'undefined') { | |||
There was a problem hiding this comment.
We talked about this in staff chat.
|
This PR is ready to land and addresses many issues in the current version of SettingGateway. Requesting review from @UnseenFaith and @bdistin. I'll write the changelog and guides once I come back home. |
kyranet
added
the
Mod: SettingsGateway
Description of the PR
This PR ports several refactors from #228 and goes further by fixing several bugs that are caused in other places. The objective of this PR is to backport some bugfixes that need to be merged quickly while improving the interface in general.
The problem with #228 is that its design has not been fully decided, and it's supposed to implement SQL improvements but goes much further than that. However, some bugfixes don't merge due to QueryBuilder and QueryType not being 100% finished nor ready for production.
Probably fixes #247
Changes Proposed in this Pull Request (List new items in CHANGELOG.MD)
Added:
Util.objectToTuplesfor object overload -> array overload inConfiguration#update.Changed:
Configuration#update.SchemaFolder#getSQLtoSchemaFolder#sqlSchema.SchemaPiece#sql's type from[string, string]tostring.datafromnullto{}, allowing object spread to attach the id of the executed task inTask#run.Configuration#update.Gateway#getPathto takepiece: nullas an option for mixed output.GatewayStorageto not depend on being inherited.Util.isClassandUtil.isObjectfor a ~5 times performance boost.Util.makeObjectto take an object as third parameter, allowing SettingGateway's internals to append properties withoutObject.assign.Removed:
Gateway#getEntry(mixed getEntry and insertEntry intoGateway#get),Gateway#createEntry, andGateway#insertEntry.Gateway#optionsandGateway#defaultSchema.Provider's nice option. As it was only used by the JSON provider. Should use a better system instead.Fixed:
Util.deepClonetrying to iterate overWeakMaps andWeakSets.Configuration#_syncStatusresolved too early.Semver Classification