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

adding typesafety for atomWithLocation #9

Open
matannahmani opened this issue Jan 4, 2023 · 19 comments
Open

adding typesafety for atomWithLocation #9

matannahmani opened this issue Jan 4, 2023 · 19 comments

Comments

@matannahmani
Copy link

matannahmani commented Jan 4, 2023

Hey, thanks for the hard work you have been putting into this new component,
I was wondering if you ever thought about adding type safety to atomWithLocation and/or serialization
We currently use Jotai and Jotai-location on our production app (NextJS), and it's sometimes a bit confusing to remember all the query keys if it needs to be shared across multiple components.

We were wondering if it would be possible for the feature to have something like this:

type LocationQuery = {
  search?: string;
  take?: number;
  skip?: number;
}

export const locAtom = atomWithLocation<LocationQuery>({
  // seralize ?
  // deserialize ?
  // initalValue ?
});

It might be out of the scope of this library but it just makes the DX much better.
Thanks again!

P.S
some resources:
https://github.com/pbeshai/use-query-params
https://github.com/amannn/next-query-params

@dai-shi
Copy link
Member

dai-shi commented Jan 5, 2023

That's exactly why getLocation and applyLocation is provided as options.

It can be too flexible DX-wise. Suggestions are welcome.
I prefer to provide a util function like atomWithStorage's createJSONStorage instead of adding more options.
For example, in pmndrs/zustand#1463, we went from serialize/deserialize option to more flexible option.

@matannahmani
Copy link
Author

matannahmani commented Jan 5, 2023

@dai-shi
Interesting i wasn't aware of getLocation and applyLocation purpose,
Would it be possible to use them as following?

/*
   atom with typed location, will use decodeURIComponent and encodeURIComponent with JSON.stringify
    to serialize and deserialize the value
    it will also accept a generic type to specify the type of the value
*/
const atomWithTypedLocation = <T>() => {
  const atom = atomWithLocation({
    getLocation() {
      // get the value from the location
      const loc = new URLSearchParams(window.location.search);
      // get the location state, if it exists
      const state = loc.get('state');
      if (state) {
        // if it exists, parse it and return it
        return JSON.parse(decodeURIComponent(state));
      }
      return undefined;
    },
    applyLocation(location) {
      // get the location state, if it exists
      const loc = new URLSearchParams(window.location.search);
      const state = loc.get('state');
      if (state) {
        // if it exists, serialize it and set it
        loc.set('state', encodeURIComponent(JSON.stringify(state)));
      } else {
        // if it doesn't exist, delete it
        loc.delete('state');
      }
      // set the location search
      location.searchParams = loc
    },
  })
  // missing part is adding the type to the atom
  return atom
}

if we can setup something like this and drill the type it can be amazing dx


type MyState = {
  foo: string;
  bar: number;
}

const typedLocAtom = atomWithTypedLocation<MyState>();

// option 1: use the atom, and set state using typed location (improved dx?)
const MyComponentTwo = () => {
  const [state, setState] = useAtom(typedLocAtom);
  return (
    <div>
      <button onClick={() => setState((prev) => ({
        ...prev,
        searchParams: { // this will be serialized
          foo: 'bar', // will have intelisense
          bar: 1,  // will have intelisense
        }
      }))}>Set State</button>
      {/* this should be also typed */}
      <div>{state.searchParams?.get('foo')}</div>
    </div>
  )
}

@matannahmani
Copy link
Author

matannahmani commented Jan 5, 2023

This is of course only one way to do it, but I will be honest I'm not quite sure how to drill the type into the search params.
But the ideal condition for me at least is using atomWithLocation the same way as we use atomWithHash.
Something like this~:

type QueryLoc = {
  filter: string;
  page: number;
}

const queryLocAtom = atomWithLocation<QueryLoc>({
  filter: '',
  page: 0,
})

const MyPage = () => {
  const [loc, setLoc] = useAtom(queryLocAtom)
  return (
    <div>
      <button onClick={() => setLoc({ filter: 'foo' })}>foo</button>
      <button onClick={() => setLoc({ page: 1 })}>page 1</button>
      <div>{loc.filter}</div>
      <div>{loc.page}</div>
    </div>
  )
}

@dai-shi
Copy link
Member

dai-shi commented Jan 6, 2023

If you need type safety (as I understand), just using JSON.parse is not enough, because it returns any type. You would need a type guard or some scheme validator.

It sounds like you may just want types (without safety) on search params.
getLocation/applyLocation can do that too, but you could also create a derived atom with bare atomWithLocation.

atomWithHash lies types (= no type safety) by default. On second look, your trial seems to follow the same approach.

So, a broader discussion topic is, while atomWithHash is key based, atomWithLocation returns all params at once (unless getLocation/applyLocation are provided.) We could create atomWithSearchParam(key, initialValue) if that fits better.

@dai-shi
Copy link
Member

dai-shi commented Jan 6, 2023

(Hm, it can actually be a good idea. It would be nice to write an RFC in https://github.com/pmndrs/jotai/discussions for more attention.)

@matannahmani
Copy link
Author

Perfect, I think types are good enough for now maybe we can set it up in an extendable way (Zod/yup 🤔)
atomWithSearchParam sounds about right, but one question that this brings to the table are we going to follow a common convention where you split the state into many params in case of an object or use JSON stringify with encoding?
eg:

type QueryLoc = {
  filter: string;
  page: number;
}

Common Way: https://jotai.org/?filter='foo'&page=8
Hash Way: https://jotai.org/query='%7B%22filter%22%3A%22foo%22%2C%22page%22%3A8%7D'
Hash Way is encodeURIComponent(JSON.stringify(*object state*))

Overall I will definitely be up to creating RFC for this and might even give it a spin this weekend.
Also, one thing to note is if we follow the hash way, where we stick the entire object into a key we can basically copy-paste almost the entire of atomWithHash code (can we ?)

@dai-shi
Copy link
Member

dai-shi commented Jan 6, 2023

one question that this brings to the table are we going to follow a common convention where you split the state into many params in case of an object or use JSON stringify with encoding?

Yeah, that's my concern too. At least, it's how atomWithHash is designed. So, we have two options on this: a) one atom for one param or b) one atom for all params.

So, this is a big discussion. We can go either way, or both, for location and hash.

Nice to hear that you are interested in writing an RFC. Looking forward to it. Yeah, you can also play with the code.

I actually want to re-implement atomWithHash without using atomWithStorage. If you are interested, you can tryy it to learn things.

@matannahmani
Copy link
Author

@dai-shi
Great I will give it a spin today, and after playing with it for a bit I will open a discussion.
I think it will be easier to implement one atom per property but one concern I have is how should we handle frequent updates and concurrent updates (let's say two parameters are updated at the same time), I feel like this can lose the state sometimes?

@dai-shi
Copy link
Member

dai-shi commented Jan 8, 2023

So, for example, if someone does this?

const atom1 = atomWithHash('theSameKey', 'foo')
const atom2 = atomWithHash('theSameKey', 'bar')

It would cause a trouble.

Or, do you mean two different keys?

const atom3 = atomWithHash('fooKey', 'foo')
const atom4 = atomWithHash('barKey', 'bar')

I think it's fine, because updates are sync. But just guessing.

@dai-shi
Copy link
Member

dai-shi commented Jan 8, 2023

Well, current atomWithLocation and new atomWithSearchParam might conflict, if used together. Not 100% sure.

@matannahmani
Copy link
Author

@dai-shi well I've done some research during the week and I think i found a few problems that this new atom raises atomWithSearchParam.
When you move to a new page that has query parameters atom, the browser back button functionality breaks. (at least on NextJS).
Other than that it seems to be somewhat working.
I will try to write RFC this weekend when i find time and maybe drop an example of how it can be integrated

@dai-shi
Copy link
Member

dai-shi commented Jan 13, 2023

the browser back button functionality breaks.

Doesn't it break with current atomWithLocation?

I will try to write RFC this weekend when i find time and maybe drop an example of how it can be integrated

👍

@matannahmani
Copy link
Author

@dai-shi
It also partially breaks with current atomWithLocation,
Takes about 3 clicks, and sometimes it completely glitch, I'm not exactly sure what's the reason.
If you're interested I can try to create a codesandbox to reproduce it.

@dai-shi
Copy link
Member

dai-shi commented Jan 13, 2023

Yeah, and I'm actually interested in your trial to fix it. 😄

@matannahmani
Copy link
Author

@dai-shi I've added a sandbox reproduction of a simple use-case (there are comments at every component)
To fix this issue I guess we have to initialize all params at once (either on href link or location atom)
Also, take a look at direct hitting a URL it will get a server hydration problem if using SSR (on the server there is no location, but the client does have it)
here is a link for sandbox

My attempts to fix this issues were fruitless (only solution is using NoSSR component or pre-initializing atom with data)

@matannahmani
Copy link
Author

matannahmani commented Jan 15, 2023

@dai-shi you can also check atomWithSearch (it's very vague and not bulletproof yet, a few bugs exist)
It's a way to serialize and deserialize atoms using encodeURIComponent and decodeURIcomponent.
I add a basic implementation that might lead to RFC in the future.
The performance is alright if you use jotai-optics and setAtom but probably it requires a better deep merge (i just took one from StackOverflow ^^) and maybe optimized caching

@dai-shi
Copy link
Member

dai-shi commented Jan 16, 2023

I'm not sure if I follow. there are two topics? a) a bug in atomWithLocation and new atomWithSearch proposal?
What should I look at?
The sandbox link seems broken.

@matannahmani
Copy link
Author

@dai-shi
can you try this link: sandbox
User 1,2 is using atom with location and custom hooks
and User 3 is using a new util atom
Correct there are currently two topics,
a)

  • there is a problem when using SSR in combination with atomWithLocaction
  • there is a problem when redirecting to a page without full query params on initialization,
    for example, you redirect to page user?id=1 and this page has additional params example sortOrderDate=desc
    sortProductName=desc, it will cause one or more pushState events to occur therefore resulting in 1+ back press to actually navigate to the previous page
    b)
  • I added a new component that is a basic implementation of an atom with query (atomWithSearch)
  • this very rough sketch for an atom that serializes its value to query params using URI encoding and JSON.stringify
  • I was a bit lazy with the typescript -sorz-

@dai-shi
Copy link
Member

dai-shi commented Jan 19, 2023

can you try this link: sandbox User 1,2 is using atom with location and custom hooks and User 3 is using a new util atom

the sandbox errors:
image

but i'm looking at the code.

Correct there are currently two topics,
a)

  • there is a problem when using SSR in combination with atomWithLocaction

Yeah, I didn't handle it. Good to know that there's a problem.

  • there is a problem when redirecting to a page without full query params on initialization,
    for example, you redirect to page user?id=1 and this page has additional params example sortOrderDate=desc
    sortProductName=desc, it will cause one or more pushState events to occur therefore resulting in 1+ back press to actually navigate to the previous page

Ah, sounds like a bug. Do you know how to fix it?

b)

  • I added a new component that is a basic implementation of an atom with query (atomWithSearch)

Is it mostly similar to what we discussed as atomWithSearchParam?

  • this very rough sketch for an atom that serializes its value to query params using URI encoding and JSON.stringify

Hm, we want something straightforward, or otherwise make things customizable. The implementation seems tricky, but that's not very important for now.

export const userAtom = atomWithSearch('state', {
  id: 3,
  date: new Date().toISOString(),
});

Oh, does that mean atomWithSearch holds all params, unlike atomWithSearchParam? To be honest, I'm not sure which is good, thus RFC might be good.

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

2 participants