Skip to content

Conversation

@jjnp
Copy link
Contributor

@jjnp jjnp commented Jun 2, 2024

Adds a fully compatible custom store that persists all values of the writable to local storage.
Caveat is of course that the value has to be serializable with JSON. An alternative might be to use devalue?

@jjnp jjnp requested a review from benjaminstrasser June 2, 2024 16:08
@jjnp jjnp requested a review from mledl as a code owner June 2, 2024 16:08
@jjnp jjnp requested a review from benjaminstrasser June 3, 2024 07:36
Co-authored-by: Benjamin Strasser <55442329+benjaminstrasser@users.noreply.github.com>
@jjnp jjnp force-pushed the localstorage-writable branch from 894482f to eb13488 Compare June 3, 2024 07:38
Copy link
Member

@benjaminstrasser benjaminstrasser left a comment

Choose a reason for hiding this comment

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

I wrote some unit tests, which also identified an issue, where localStorageWritable(key, defaultValue) adds the value to the store but does not insert the default value into localstorage.

import { localStorageWritable } from './localstorage-writable'
import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'
import { get } from 'svelte/store'

const mockedBrowser = vi.hoisted(() => {
	return vi.fn()
})

vi.mock('$app/environment', () => {
	return {
		get browser() {
			return mockedBrowser()
		}
	}
})

const localStorageMockFactory = () => {
	let store: Record<string, string> = {}
	return {
		getItem: (key: string) => store[key] || null,
		setItem: (key: string, value: string) => {
			store[key] = value
		},
		removeItem: (key: string) => {
			delete store[key]
		}
	}
}

describe('localStorageWritable', () => {
	beforeEach(() => {
		mockedBrowser.mockReturnValue(true)
		vi.stubGlobal('localStorage', localStorageMockFactory())
	})

	afterEach(() => {
		vi.resetAllMocks()
	})

	it('should initialize with default value if no value in localStorage', () => {
		const defaultValue = 'default'

		const store = localStorageWritable('testKey', defaultValue)

		expect(get(store)).toBe(defaultValue)
		expect(localStorage.getItem('testKey')).toBe(JSON.stringify(defaultValue))
	})

	it('should initialize with value from localStorage if present', () => {
		localStorage.setItem('testKey', JSON.stringify('storedValue'))

		const store = localStorageWritable('testKey')

		expect(get(store)).toBe('storedValue')
	})

	it('should prioritize value from local storage over default', () => {
		localStorage.setItem('testKey', JSON.stringify('storedValue'))

		const store = localStorageWritable('testKey', 'defaultValue')

		expect(get(store)).toBe('storedValue')
		expect(localStorage.getItem('testKey')).toBe(JSON.stringify('storedValue'))
	})

	it('should store value in localStorage when set', () => {
		const store = localStorageWritable('testKey')
		store.set('newValue')

		expect(localStorage.getItem('testKey')).toBe(JSON.stringify('newValue'))
		expect(get(store)).toBe('newValue')
	})

	it('should remove value from localStorage when wipe is called', () => {
		localStorage.setItem('testKey', JSON.stringify('storedValue'))

		const store = localStorageWritable('testKey')
		store.wipe()

		expect(localStorage.getItem('testKey')).toBe(null)
		expect(get(store)).toBe(null)
	})

	it('should update the value correctly using update method', () => {
		const store = localStorageWritable<number>('testKey', 1)
		store.update((n) => n! + 1)

		expect(localStorage.getItem('testKey')).toBe(JSON.stringify(2))
		expect(get(store)).toBe(2)
	})
})

describe('localStorageWritable with no browser', () => {
	beforeEach(() => {
		mockedBrowser.mockReturnValue(false)
		vi.stubGlobal('localStorage', undefined)
	})

	afterEach(() => {
		vi.resetAllMocks()
	})

	it('should initialize with default value', () => {
		const defaultValue = 'default'
		const store = localStorageWritable('testKey', defaultValue)

		expect(get(store)).toBe(defaultValue)
	})

	it('should initialize with null', () => {
		const store = localStorageWritable('testKey')

		expect(get(store)).toBe(null)
	})

	it('should set value', () => {
		const store = localStorageWritable('testKey')
		store.set('newValue')

		expect(get(store)).toBe('newValue')
	})

	it('should remove value', () => {
		const store = localStorageWritable('testKey')
		store.set('newValue')
		store.wipe()

		expect(get(store)).toBe(null)
	})

	it('should update the value correctly using update method', () => {
		const store = localStorageWritable<number>('testKey', 1)
		store.update((n) => n! + 1)

		expect(get(store)).toBe(2)
	})
})

@sjaghori
Copy link
Collaborator

sjaghori commented Jun 3, 2024

For what kind of use cases are we going to utilize this?

@benjaminstrasser
Copy link
Member

benjaminstrasser commented Jun 3, 2024

For what kind of use cases are we going to utilize this?

I think it is intended for #77

@benjaminstrasser benjaminstrasser mentioned this pull request Jun 3, 2024
@jjnp
Copy link
Contributor Author

jjnp commented Jun 4, 2024

@benjaminstrasser Pls directly push the unit-tests, I'll look at the issue (but ofc feel free to add a fix directly, if you have one)

@jjnp
Copy link
Contributor Author

jjnp commented Jun 4, 2024

For what kind of use cases are we going to utilize this?

Right, it's intended for the other PR, but it comes in handy pretty often. Might be some things and local configs like dark mode settings, preferences, undo history, recent search keywords, ...

Signed-off-by: Benjamin Strasser <bp.strasser@gmail.com>
Signed-off-by: Benjamin Strasser <bp.strasser@gmail.com>
@benjaminstrasser benjaminstrasser force-pushed the localstorage-writable branch from 54a4f33 to f418270 Compare June 4, 2024 16:16
Copy link
Contributor

@mledl mledl left a comment

Choose a reason for hiding this comment

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

Nice utility and good testing 🚀

@benjaminstrasser benjaminstrasser merged commit a470ca6 into main Jun 4, 2024
@benjaminstrasser benjaminstrasser deleted the localstorage-writable branch June 4, 2024 16:36
const storedValue = browser ? localStorage.getItem(localStorageKey) : null
const localStorageValue = (() => {
try {
return storedValue !== null ? JSON.parse(storedValue) : null
Copy link
Collaborator

Choose a reason for hiding this comment

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

@benjaminstrasser the check here is redundant.
JSON.parse(null) === null

Copy link
Member

Choose a reason for hiding this comment

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

Yes you are correct! I caught that too. Problem is JSON.parse is typed as parse(value: string) and not parse(value: string | null) even though parse(null) === null.

See here: #81 (comment)

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.

5 participants