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

Read data from disk once #184

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

fablee1
Copy link

@fablee1 fablee1 commented Sep 7, 2023

Each read right now reads the data from the disk, which affects performance if conf is used in a high load application as the main storage.
This change stores each write additionally in the state and on each use of the store it deep clones the state and returns it instead of reading from disk. Deep cloning is needed to prevent the mutation of the state variable and ensure conf fully behaves as previously.
Speed improvements for reads is 100-500x, especially if encryption is used.

@@ -60,6 +60,8 @@ export default class Conf<T extends Record<string, any> = Record<string, unknown
readonly #encryptionKey?: string | Buffer | NodeJS.TypedArray | DataView;
readonly #options: Readonly<Partial<Options<T>>>;
readonly #defaultValues: Partial<T> = {};
#readFromDisk = true;
#state?: string;
Copy link

@antongolub antongolub Oct 9, 2023

Choose a reason for hiding this comment

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

I support this PR, but I'd like to suggest a few improvements, if you don't mind.

  1. To avoid a breaking change introduce a new option like memoize/useMemoryState
get store(): T {
		const state = createPlainObject();

		if (this.options.memoize) {
		   if (this.#state) {
		      return this.#state;
		   }

		   this.#state = state;
		}
		
		try {
			const data = fs.readFileSync(this.path, this.#encryptionKey ? null : 'utf8');
			const dataString = this._encryptData(data);
			const deserializedData = this._deserialize(dataString);
			this._validate(deserializedData);
			
			return Object.assign(state, deserializedData);
		} catch (error: unknown) {
			if ((error as any)?.code === 'ENOENT') {
				this._ensureDirectory();
				return state;
			}
			if (this.#options.clearInvalidConfig && (error as Error).name === 'SyntaxError') {
				return state;
			}
			throw error;
		}
	}
  1. Use change event to trigger state reset instead of _watch patch
if (options.memoize) {
  this.events.on('change', this._resetState)
}
// ...
_resetState() {
   this.#state = undefined;
}

Choose a reason for hiding this comment

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

@sindresorhus,

Could you plz take a look?

Copy link
Owner

Choose a reason for hiding this comment

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

I agree with the suggestions here. It should be behind an option. We can make the option true by default at some point when it's more battle-tested.

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.

3 participants