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

withDefaults should (optionally) populate the URL #405

Open
tordans opened this issue Nov 17, 2023 · 4 comments
Open

withDefaults should (optionally) populate the URL #405

tordans opened this issue Nov 17, 2023 · 4 comments

Comments

@tordans
Copy link
Contributor

tordans commented Nov 17, 2023

(Extracted from #404 (comment))

Right now, withDefaults does set the internal state but it does not populate the URL.
The URL is only first populated once an explicit setConfig (for useQueryState('config')) is called.

There are minor issues with this approach:

  • If it where to populate the URL, the order in which the app first sets up useQueryState is also the order in which the URL params are sorted, which is great for SEO and readability of the URL
  • A user can always share the non-param URL, however the state of the application is then not preserved in the URL. Using the same URL later would result in a different UI if the defaults changed in the meantime.
  • If the URL is present, it signals to the User right from the start that this URL can be shared and will preserve the UI
  • The user only sees a change in the URL at the start of the application, not (unexpectedly) on first interaction

For my usecase, it would be perfect if setting the URL where an option on withDefaults. With this I could decide per param if I want this to be visible in the URL right away or not. I have params that would ideally be visible right away and some that should only show once a user interacts with a less used feature.

@Talent30
Copy link
Contributor

Talent30 commented Nov 17, 2023

I don't think it's a good idea, and I don't think this is a bug. This feature is working as it's intended to be.

If it where to populate the URL, the order in which the app first sets up useQueryState is also the order in which the URL params are sorted, which is great for SEO and readability of the URL

Not sure about SEO, I don't think the params in URL are designed for readability.

A user can always share the non-param URL, however the state of the application is then not preserved in the URL. Using the same URL later would result in a different UI if the defaults changed in the meantime.
If the URL is present, it signals to the User right from the start that this URL can be shared and will preserve the UI

If the state has default value, then the user doesn't even need to care if the state exists on param or not.

The existence of a default value represents the default state expected by the programme designer

The user only sees a change in the URL at the start of the application, not (unexpectedly) on first interaction

Because they have changed the default state of the application.

I have params that would ideally be visible right away and some that should only show once a user interacts with a less used feature.

Such behavior is divisive, this goes against your previous thoughts.

@franky47
Copy link
Member

franky47 commented Nov 17, 2023

I don't think it's a good idea, and I don't think this is a bug. This feature is working as it's intended to be.

I agree in not calling this a bug, but it's not really a feature either.

There is definitely something missing in the way things are implemented at the moment, and that is distinguishing a missing search param from an invalid one.

At the moment, parsers only have one option: return null when they can't correctly turn a query string value into the underlying JS type. But null is also used to indicate that the query key is missing altogether.

The default value was slapped onto the problem to help with type safety, ie to ensure there never was a nullish (null | undefined) value in app code, to simplify logic.

But this is clearly causing confusion and not allowing a proper distinction between two very different cases. This will be made even more apparent when introducing validation in parsers (eg with Zod).

@franky47 franky47 removed the bug Something isn't working label Nov 17, 2023
@Talent30
Copy link
Contributor

I don't think it's a good idea, and I don't think this is a bug. This feature is working as it's intended to be.

I agree in not calling this a bug, but it's not really a feature either.

There is definitely something missing in the way things are implemented at the moment, and that is distinguishing a missing search param from an invalid one.

At the moment, parsers only have one option: return null when they can't correctly turn a query string value into the underlying JS type. But null is also used to indicate that the query key is missing altogether.

The default value was slapped onto the problem to help with type safety, ie to ensure there never was a nullish (null | undefined) value in app code, to simplify logic.

But this is clearly causing confusion and not allowing a proper distinction between two very different cases. This will be made even more apparent when introducing validation in parsers (eg with Zod).

In my understanding it has always been a feature for type safety and null value safety.

Do we need to make a distinction between them or just simply rename it as safeFallbackValue?

@tordans
Copy link
Contributor Author

tordans commented Nov 17, 2023

There is definitely something missing in the way things are implemented at the moment, and that is distinguishing a missing search param from an invalid one.

Its less about invalid params for me, but about how to handle the initial state. With const [a, setA] = useState('foo') the initial state is just as important as being able to read and set it.

From a meta level of view, I notices that I still have to re-align my mental model of the library as "useState-first and URL second". (Which is also why I just created #408 to talk about clarifying this.)

From a feature point of view, it would be great to have the option to use withDefault to push the default to the URL whenever it is missing.


Also, feel free to move this ticket into the Ideas section.

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