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

setKeys method on store #110

Open
shiftlabs1 opened this issue May 25, 2022 · 14 comments
Open

setKeys method on store #110

shiftlabs1 opened this issue May 25, 2022 · 14 comments

Comments

@shiftlabs1
Copy link

shiftlabs1 commented May 25, 2022

First off, Thank you guys for this library. I wanted to ask if you can possibly include a setKeys (on the map store) method that internally simply does

const setKeys = (partialState : Partial<state>)=>{
     store.set({...store.get(),...obj});
}

or in the map store define this function

  store.setKeys=function(object){
    Object.keys(object).forEach(key=>{
      store.setKey(key,object[key]);
    })
  }

or a function that also handles deeply nested objects (like lodash-es merge method).
I can't say how performant or not the second option is, but it leverages the checks done in setKey method.
Or if there is a way to create an extension on the store that adds this, kindly point that to me. This will help reduce overall the lines of code one has to write as more often that not, we are partially updating the store , and setKey only does a key at a time.

@ai
Copy link
Member

ai commented May 25, 2022

Our current policy is to use helper functions to all syntax sugar. Something like: setKeys(map, { a: 1, b: 2 }) rather than method map.setKeys({ a: 1, b: 2 }).

Without this policy, we will not be able to be nano library with extremely small footprint.

Is it OK for you? We can add setKeys helper to the library as additional export.

@hgenru
Copy link

hgenru commented Feb 4, 2023

@ai there is one interesting thing: store.get() triggers onMount. Accordingly, to change the store via ...rest, you must first get the previous state via store.get(), which can cause unexpected behavior.

Or it might push to use store.setKey() in loop, which will cause unnecessary state updates.

@ai
Copy link
Member

ai commented Feb 4, 2023

you must first get the previous state via store.get(), which can cause unexpected behavior.

I disagree of “unexpected behavior.”

For instance, localStorage’s store remove listener if nobody use the store and store.value can be outdated on next setKey call. We are calling onMount to update the current value.

@gregg-cbs
Copy link

gregg-cbs commented Mar 29, 2023

*Update: I found the map store: cart = map<Cart>({}) which has setKey which is singular. So it half solves what I was explaining below.


I think doing this everywhere is crazy:

cart.set({
  ...cart.get(),
  ...someUpdatedData,
  is_loading: true
})

For developer experience this is not great at all.

I cannot see why you do not have a method that allows updates, so we can do this:

cart.update({key: 'value'})
// or
cart.update({key: 'value', key2: 'value2'})
// or
cart.update(someUpdatedData)

That is more natural then .setKey() and .set().

I understand you are talking about nanostores footprint but come on, the above request is standard coding practice that every developer will use. My store is bloated is .set(), .get(), ...destructure and the bloat in my own code alone from your design decision far outweighs the 5 lines of code you need to add to create an update method.

@blikblum
Copy link

https://stackblitz.com/edit/vitejs-vite-qjgnio?file=main.js&terminal=dev

I have a similar issue

I created a map and listened to a key ('isLogged')

const session = map({ isLogged: false, isLogging: false, user: undefined });

session.listen((value, key) => {
  if (key === 'isLogged') {
    console.log('isLogged', value.isLogged);
  }
});

Using session.setKey('isLogged', true);, triggers the listener with key === 'isLogged' as expected

When a reset the store using set : session.set({ isLogged: false, isLogging: false, user: undefined }); key is undefined.

I could check for undefined also but it can occurs to trigger the change even if the desired key does not change

const session = map({ isLogged: false, isLogging: false, user: undefined });
session.listen((value, key) => {
  if (key === 'isLogged' || key === undefined) {
    console.log('isLogged', value.isLogged);
  }
});

//  console.log will be called 3 times even if 'isLogged' does not change
session.set({ isLogged: false, isLogging: false, user: undefined });
session.set({ isLogged: false, isLogging: true});
session.set({ isLogged: false, isLogging: true, user: 'xxx'});

An API like map#reset that trigger appropriate change events for each key changed would allow to do a more fined tracking

@ai
Copy link
Member

ai commented May 25, 2023

@blikblum you can use listenKeys() helper to clean up code.

import { listenKeys } from 'nanostores'

listenKeys(session, ['isLogged'], () => {
  console.log('isLogged', value.isLogged)
})

console.log will be called 3 times even if 'isLogged' does not change

Changed key is just a small optimization. It is a way to reduce callbacks call a little.

Do you have a real use case, when callback have to be called only on value changed?

@blikblum
Copy link

I've hit the described issue (listening to a key not triggering when store is updated) twice:

I load the app config when store is started and set to a map store. The idea is to have appConfig store as source of truth and derives other config stores like insurances from it

const initialValue = {}

export const appConfig = map(initialValue)

export const updateConfig = action(appConfig, 'updateConfig', (store, { key, value }) => {
  store.setKey(key, value)
})

let configLoaded = false

onStart(appConfig, async () => {
  if (!configLoaded) {
    const platform = await getPlatform()
    const dir = platform === 'win32' ? BaseDirectory.LocalData : BaseDirectory.Config
    const configFileName = await join('auto-fisio-invoice', 'config.json')
    const configExists = await exists(configFileName, {
      dir,
    })
    if (configExists) {
      const content = await readTextFile(configFileName, {
        dir,
      })
      const appConfigData = JSON.parse(content)
      appConfig.set(appConfigData)
      configLoaded = true
    }
  }
})

Then i listen to insurances key and update a second store:

export const insurances = atom([])

appConfig.listen((value, key) => {
  if (key === 'insurances' || key === undefined) {
    setInsurances(value.insurances)
  }
})

export const setInsurances = action(insurances, 'setInsurances', (store, value) => {
  store.set(value)
  return value
})

The first time i wrote the code, i did not added the key === undefined check and the insurances store was not initialized.

My reasoning is that the 'insurances' value changed after loading the data so i was expecting to listener be called with each key whose value changed

@ai
Copy link
Member

ai commented May 26, 2023

My reasoning is that the 'insurances' value changed after loading the data so i was expecting to listener be called with each key whose value changed

It will be very bad for performance.

Key listening is a rare feature which is not very affective (better for optimization is to create a computed store).

@blikblum
Copy link

My reasoning is that the 'insurances' value changed after loading the data so i was expecting to listener be called with each key whose value changed

It will be very bad for performance.

Key listening is a rare feature which is not very affective (better for optimization is to create a computed store).

Performance here is not an issue since is triggered once in a while. DX is more important which seems the whole point of map (avoid boilerplate destructuring when setting a property of a store)

@ai
Copy link
Member

ai commented May 26, 2023

@blikblum the way for better DX is just not using listenKeys and key argument in callback. It is microoptimization.

@blikblum
Copy link

There's another use case (i think i had posted earlier) that i hit when using map

A auth session store

import { GoogleAuthProvider, getAuth, onAuthStateChanged, signInWithPopup } from 'firebase/auth'
import { session } from '../stores/session.js'

const auth = getAuth()

export function startSession() {
  session.setKey('isLogging', !!localStorage.getItem('hasCredential'))

  onAuthStateChanged(auth, async (user) => {
    if (user) {
      localStorage.setItem('hasCredential', true)
      const { displayName: name, uid: id, email } = user
      session.setKey('user', { id, name, email })
      session.setKey('isLogged', true)
      session.setKey('isLogging', false)
    } else {
      localStorage.removeItem('hasCredential')
      session.set({ isLogged: false, isLogging: false, loginError: undefined, user: undefined })
    }
  })
}

export async function signIn() {
  session.setKey('isLogging', true)
  try {
    const provider = new GoogleAuthProvider()
    provider.setCustomParameters({
      prompt: 'select_account',
    })
    await signInWithPopup(auth, provider)
  } catch (error) {
    session.setKey('isLogged', false)
    session.setKey('isLogging', false)
    session.setKey('loginError', {
      title: 'Erro ao autenticar usuário',
      content: `${error}`,
    })
  }
}

export async function signOut() {}

I wrote a listener to isLogged state to update ui accordingly

export function onLogged(callback) {
  session.subscribe((value, key) => {
    if (key === 'isLogged' ) {
      callback(value)
    }
  })
}

But there's a bug at line: session.set({ isLogged: false, isLogging: false, loginError: undefined, user: undefined }) . The callback will not be called.

If i want to keep setKey functionality i have to change the reset code to:

session.setKey('isLogged', false)
session.setKey('isLogging', false)
session.setKey('user',undefined)
session.setKey('loginError',undefined)

@marcebdev
Copy link

marcebdev commented Oct 23, 2023

I would advocate for a setKeys function as well, reason being if we have to do (or even if it seems compelling to do) something like Object.assign(myMap.get(), { a: 1, b: 2 }) or { ...myMap.get(), a: 1, b: 2 } then why use an map over an atom at all?

Presumably one of the great parts of this library is map & deepMap achieve what Recoil or Jotai (since the recent update) do with atomFamily but in a way that is more biased & possibly less flexible (since atom family allows custom equality comparison func) however is simpler and likely sufficient and more adept to most use cases.

I believe that when we reach a point where a small API addition prevents the library from being used it's critical to add that extra functionality.

On another note, I would imagine the requests in regards to listenKey/computed probably align with it'd be interesting to see if we could get functionality closer to selectAtom or what benefits that may have, but I think that gets out of the scope of this particular issue.

@shiftlabs1
Copy link
Author

Our current policy is to use helper functions to all syntax sugar. Something like: setKeys(map, { a: 1, b: 2 }) rather than method map.setKeys({ a: 1, b: 2 }).

Without this policy, we will not be able to be nano library with extremely small footprint.

Is it OK for you? We can add setKeys helper to the library as additional export.

Hi @ai . Did you eventually add the helper function "addKeys" to the library ? In the interest of keeping the library as light weight , It would be nice to have this .

@ai
Copy link
Member

ai commented Nov 10, 2023

Did you eventually add the helper function "addKeys" to the library ?

No, feel free to open PR

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

No branches or pull requests

6 participants