Skip to content

Conversation

@dmaskasky
Copy link
Collaborator

@dmaskasky dmaskasky commented Oct 8, 2025

Related Bug Reports or Discussions

#3164

Fixes #

Summary

I don't think we should merge this PR. I don't think Jotai should support side-effects in atom read.

The use-case which spawned this investigation was with

function atomShallowEqual<T>(fn: (get: Getter) => T): Atom<T> {
  const previousValueAtom = atom<T | void>();
  return atom((get) => {
    const jotaiStore = getDefaultStore();
    const oldValue = jotaiStore.get(previousValueAtom);
    const newValue = fn(get);
    jotaiStore.set(previousValueAtom, newValue);
    if (!oldValue) {
      return newValue;
    }
    return shallowEquals(newValue, oldValue) ? oldValue : newValue;
  });
}

I think there could be a better pattern for above. I haven't found a pattern that supports side-effects in read.

Check List

  • pnpm run fix for formatting and linting code and docs

@vercel
Copy link

vercel bot commented Oct 8, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
jotai Ready Ready Preview Comment Oct 8, 2025 10:43pm

@dmaskasky dmaskasky marked this pull request as draft October 8, 2025 22:37
@codesandbox-ci
Copy link

codesandbox-ci bot commented Oct 8, 2025

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

@pkg-pr-new
Copy link

pkg-pr-new bot commented Oct 8, 2025

More templates

npm i https://pkg.pr.new/jotai@3165

commit: f5edd30

@github-actions
Copy link

github-actions bot commented Oct 8, 2025

LiveCodes Preview in LiveCodes

Latest commit: f5edd30
Last updated: Oct 8, 2025 10:43pm (UTC)

Playground Link
React demo https://livecodes.io?x=id/GCEZ9WQ3E

See documentations for usage instructions.

@dmaskasky dmaskasky force-pushed the read-side-effect-test branch from 32650fe to f5edd30 Compare October 8, 2025 22:43
@dmaskasky dmaskasky marked this pull request as ready for review October 8, 2025 22:43
@dmaskasky
Copy link
Collaborator Author

/ecosystem-ci run

@github-actions
Copy link

github-actions bot commented Oct 8, 2025

Ecosystem CI Output

---- Jotai Ecosystem CI Results ----
{
  "bunshi": "PASS",
  "jotai-devtools": "PASS",
  "jotai-effect": "FAIL",
  "jotai-scope": "PASS",
  "waku-jotai": "PASS"
}

@dmaskasky dmaskasky requested a review from dai-shi October 8, 2025 22:54
@dmaskasky
Copy link
Collaborator Author

"jotai-effect": "FAIL",

I expected this.

@dai-shi dai-shi marked this pull request as draft October 9, 2025 06:33
@dai-shi
Copy link
Member

dai-shi commented Oct 9, 2025

(I'll have a look later.)
(Sorry for the delay, not on it yet.)

Copy link
Member

@dai-shi dai-shi left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. I agree. This shouldn't be merged.

const previousValueAtom = atom()
const testAtom = atom((get) => {
const newValue = get(basicAtom)
store.set(previousValueAtom, newValue)
Copy link
Member

Choose a reason for hiding this comment

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

We don't need to support this.
If we can't support this, we should probably warn it (dev only) if we can detect it.

@dai-shi
Copy link
Member

dai-shi commented Oct 31, 2025

The use-case which spawned this investigation was with

function atomShallowEqual<T>(fn: (get: Getter) => T): Atom<T> {
  const previousValueAtom = atom<T | void>();
  return atom((get) => {
    const jotaiStore = getDefaultStore();
    const oldValue = jotaiStore.get(previousValueAtom);
    const newValue = fn(get);
    jotaiStore.set(previousValueAtom, newValue);
    if (!oldValue) {
      return newValue;
    }
    return shallowEquals(newValue, oldValue) ? oldValue : newValue;
  });
}

My suggestion is something like this:

function atomShallowEqual(initialValue) {
  const valueAtom = atom(initialValue);
  return atom(
    (get) => get(valueAtom),
    (get, set, newValue) => {
      const oldValue = get(valueAtom);
      if (!shallowEqual(oldValue, newValue)) {
        set(valueAtom, newValue);
      }
    }
  );
}

So, the challenge is how we can do with fn: (get: Getter) => T.
If the requirement is only about shallowEqual, and if we only need to support one jotai store, we could simply use a scope variable.

function atomShallowEqual(fn) {
  let lastValue;
  return atom((get) => {
    const value = fn(get);
    if (!shallowEqual(lastValue, value)) {
      lastValue = value;
    }
    return lastValue;
  });
}

It still violates purity, but it feels like a better hack.

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.

2 participants