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

Clear() and Defaults #138

Open
willium opened this issue Jan 14, 2021 · 0 comments
Open

Clear() and Defaults #138

willium opened this issue Jan 14, 2021 · 0 comments

Comments

@willium
Copy link

willium commented Jan 14, 2021

With this fix landed, a new bug in clear() has become evident.
As you know, settings .store = ... emits a change event.

The code for clear() is currently as follows:

clear(): void {
	this.store = createPlainObject(); // <-- set store to an empty object, which will in turn emit a change event where current = {}

	for (const key of Object.keys(this.#defaultValues)) {
		this.reset(key); // <-- reset keys, without emitting a change event
	}
}

As you can see, we emit a change event with a blank object, and don't properly emit the post-reset() store.


Noticing this illuminated the following static typing predicament:

suppose we initialize a Conf:

type MyType = { foo: number }
const conf = new Conf<MyType>() // <-- this should require a defaults property with a value of type MyType

Instead of following this, the internal implementation basically always assumes that defaults is a Partial<T> and that the Conf, despite its type being MyType, is actually only Partial<T>. For practical use and type safety, the Conf should statically type to T (MyType in my example), not Partial<T>. If not, and you want to stick with Partial<T> it's critical that

get store()'s method header is rewritten to

get store(): Partial<T>

because either following a reset() or from the very onset with a partial defaults object, the client cannot be sure that the store is of type T.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants