Fix .clear() behavior
#136
Conversation
Fixes #135
source/index.ts
Outdated
| */ | ||
| clear(): void { | ||
| const keys = Object.keys(this.store); |
There was a problem hiding this comment.
If the current value of the store has a key deleted that is in the defaults or schema, I believe it wouldn't be included in this and you'd end up with an incomplete default.
|
|
||
| for (const key of keys) { | ||
| this.reset(key); | ||
| } |
There was a problem hiding this comment.
I think it'd make sense to emit the change event here, what do you think?
this.events.emit('change');
There was a problem hiding this comment.
That is already done, since we set this.store:
Line 344 in 43c644f
There was a problem hiding this comment.
@sindresorhus this is actually causing a regressive bug in my program. While we set this.store, we do it before reseting the parameters, and we set it to a blank object—which disobeys the type contract. Subsequently it's emitting an onDidAnyChange event where the current value is undefined (which disobeys the type guarantees for the store, given defaults)
There was a problem hiding this comment.
this doesn't cause a static typing violation because of the use Partial in several places where I don't believe it makes sense. Ideally it'd be possible to create a Conf with strict type guarantee. The type of details should statically guaranteed to be the base type.
There was a problem hiding this comment.
I don't have the time to look into this right now, so it would be good if you could open an issue instead. Even better if you could also submit some failing tests.
There was a problem hiding this comment.
I created an issue. The type safety bug here is pretty important. The clear() bug is as well, given that is breaks the contract with the client regarding types.
|
excited to see this land! only remaining concern is that this is probably a breaking change for people who rely on public clear(restoreDefaults?: boolean): voidso that people like me can just pass |
|
It's really just a bug fix, but I will do a major release just to be careful. |
Fixes #135