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

atomWithLocation initVaule is not sync #37

Open
WeiShengv99 opened this issue Aug 8, 2024 · 5 comments
Open

atomWithLocation initVaule is not sync #37

WeiShengv99 opened this issue Aug 8, 2024 · 5 comments

Comments

@WeiShengv99
Copy link

https://stackblitz.com/edit/github-3vpjew?file=src%2FApp.tsx

  • About page has url search params, and i use atomWithLocation to get it
  • Other pages dont use atomWithLocation
  • enter Home page first is required , then go to About page
    image
    baseAtom.onMount = (set) => {
    const callback = () => set(getL());
    const unsub = sub(callback);
    callback();
    return unsub;
    };
    const derivedAtom = atom(
    (get) => get(baseAtom),
    (get, set, arg: SetStateAction<T>, atomOptions: AtomOptions<T> = {}) => {
    set(baseAtom, arg);
    appL(get(baseAtom), { ...options, ...atomOptions });
    },

maybe we can return getL() when baseAtom not mount. not just get(baseAtom

@dai-shi
Copy link
Member

dai-shi commented Aug 8, 2024

Thanks for reporting.
While it seems like a bug, it's also working as design.
I wonder if it should be fixed. If we return getL(), it may break compatibility with Concurrent React. But, I'm not very sure.

Let's see if @Flirre has an opinion.

@Flirre
Copy link
Collaborator

Flirre commented Aug 8, 2024

I'll try to take a proper look at it during the weekend 👀

@Flirre
Copy link
Collaborator

Flirre commented Aug 12, 2024

I won't have time to investigate this. On a high level it seems like a nice fix, but I'm not sure either about Concurrent React. Could that be checked with some tests?

@dai-shi
Copy link
Member

dai-shi commented Aug 12, 2024

@Flirre Thanks for the note. It's too hard to test actually.

@WeiShengv99 If I understand correctly, the issue can be hidden from the user. Let me know if you have a real issue other than performance problem or unexpected internal behavior.

If anyone is interested in this issue, feel free to jump in.

@WeiShengv99
Copy link
Author

the issue can be hidden from the user.

@dai-shi @Flirre Thank you for your work.
I dont have other issue about performance problem or unexpected internal behavior.

Yes,users can add an extra isInit state in their projects to delay read atom state, Actually, that's what I did

const innerLocationAtom = atomWithLocation({
  replace: true,
});
const isInitAtom = atom(false);
const locationAtom = ((get) => get(innerLocationAtom),(_get, set, update: boolean) => {
    set(isInitAtom, update);
  },)
locationAtom.onMount = (setAtom) => {
  setAtom(true);
  return () => {
    setAtom(false);
  };
}

If return getL() may break compatibility with Concurrent React, this issue should be handled by the users themselves.

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

3 participants