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

Add support for parsing arrays into base64 strings in the URL. #552

Open
wants to merge 2 commits into
base: next
Choose a base branch
from

Conversation

harrisrobin
Copy link

@harrisrobin harrisrobin commented May 1, 2024

Hi,
first off all, thank you for writing this lib @franky47 , it's been a real life saviour!

While running into a use case where I need to keep state in the URL, I noticed that having objects in the URL break in certain messaging apps like whatsapp, telegram and kakaotalk (that's all i've tested).

As a result, I opt-ed to store my arrays as a base64 string in the URL, which works quite well for my use-case. I don't know if this is something other people seek, but wanted to make a PR incase it's of any interest.

I tried to keep it very close to parseAsArrayOf:

  const [selectedTags, setSelectedTags] = useQueryState(
    "selectedTags",
    parseAsBase64ArrayOf(parseAsJson<TagOption>()).withDefault([]),
  )

Copy link

vercel bot commented May 1, 2024

@harrisrobin is attempting to deploy a commit to the 47ng Team on Vercel.

A member of the Team first needs to authorize it.

Copy link
Member

@franky47 franky47 left a comment

Choose a reason for hiding this comment

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

Applying some encoding before writing to the URL is a nice idea, I would argue that this approach could be made more generic than arrays, and compose with any underlying parser.

The heart of the problem is more concerning though: if URLs break on some platforms, then the URL encoding logic (kept custom to allow "pretty" URLs, see #372) should be updated instead.

Back to the base64 matter: it would indeed be a good format to wrap any kind of value and make it URL-safe (at the expense of added opacity), one issue as noted in a diff note is the lack of a Unicode-safe implementation native to all platforms.

I may accept a wrapper that doesn't pull Node.js API stubs in the browser, and can easily be tree-shaken if not used.

if (!query) return null
if (query === '') return []
try {
const decoded = Buffer.from(query, 'base64').toString('utf-8')
Copy link
Member

Choose a reason for hiding this comment

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

note: Buffer is a Node.js API, and this code needs to run both on the server and in browsers. atob is not recommended either as it doesn't support Unicode text inputs, though there are workarounds. There is a proposal for browser-native base64 encoding, but it's still in early stages.

Copy link
Author

Choose a reason for hiding this comment

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

I would love to implement a more generic approach. Do you have any ideas or opinions on what using it would look like?

re unicode-safe implementation: I can't find a good wrapper what would fit the bill, so I'll probably look into the workarounds you linked to.

Copy link
Member

@franky47 franky47 May 1, 2024

Choose a reason for hiding this comment

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

An example would be (using only atob/btoa for simplicity):

export function parseAsBase64<T>(parser: Parser<T>) {
  return createParser<T>({
    parse: query => {
      if (!query) return null
      try {
        return parser.parse(atob(query))
      } catch {
        return null
      }
    },
    serialize: value => btoa((parser.serialize ?? String)(value))
  })
}

Copy link
Member

Choose a reason for hiding this comment

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

suggestion: I need to pin the pnpm version to 9 in the package.json, could you revert this file for the time being please (and/or use pnpm 9) ?

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