querystrings in state - Githubissues

kyeotic commented 7 years ago

The docs say

Intentionally we have chosen to solely support paths since they are best for SEO and keep the API minimal. You are free to use query strings to request data in your thunks.

This is a bit hand-wavy; I think it could be more helpful to explain what the intention is for thunks.

Should they be reading the querystring from the window.location object? If so, how is SSR indtended to work with querystrings?
Is the querystring stored in an undocumented part of the reducer state? If so, it should be documented.
Are thunks supposed to access the querystring in some other way?

faceyspacey commented 7 years ago

It could and soon will be written better. Hand wavy lol. I like that.

I'm on my phone--the intent is to say u can use query strings in the URLs u ping. Not in what u extract out of state. We store NO query strings.

...sounds like I have even more of a reason to clarify.

kyeotic commented 7 years ago

I think that is really going to complicate Components that do rely on querystrings. The querystring is part of the URL and an important piece of the state of pages that rely on them, like search pages. If the goal of this library is to say "Don't use URLs, don't think about the browser location, just use your application state" then not storing the querystring means you either can't use querystring's at all or you are violating the primary tenant of the library. Neither of those seem like great options.

To take it further, say I do get the querystring from some other place (like window.location), I am probably going to break the ability to replay/rewind redux actions. Will redux-first-router correctly restore querystrings to the URL if I use the back action? What about the url hash?

I don't see how you can control the URL and but ignore querystrings; they are part of the URL.

faceyspacey commented 7 years ago

Ur gonna have to change ur thinking my brother if u wanna use this library. I have avoided query strings successfully for years. Either that or u convince me why we still need query strings. If this is just a case for legacy apps, it's prolly a no go. But if even greenfield apps need query strings, I may be convinced.

My current perspective is this:

ur client has legacy needs which involve query strings. This package likely won't support ur needs.
Ur a greenfield project, and u don't want to use query strings anyway. This package is for u.

I see ur doing stuff for Nike. U prolly need to deal with query strings from referrers. Basically incoming links likely are the only case I can see. Maybe u can see more, but I don't recommend people build modern app startups linking from page to page using query params. Use paths, they are way better for seo, shorter, cleaner, and all routing tools (this, express, React router, etc) can extract params based on positional segments.

Consider this a debate as to whether to support the feature. What use cases beyond referrers?

If it's just referrers, yea, just get it from window.location and call it a day. U can check state.location.kind === 'load' on the client to determine if it's the first page of the session.

kyeotic commented 7 years ago

Search results are the big one. If I perform two searches the back button should take me back to the first search. If the search is simple I guess you could still express that as a path component, but if the search is complex (multiple attributes) that would get difficult fast.

faceyspacey commented 7 years ago

Good use-case.

faceyspacey commented 7 years ago

My recommendation: use the lib and see if u like it. Then let's revisit this. It's one of those things where u have percolated my mind but I won't have the answer immediately and it def won't be a feature this or next week.

kyeotic commented 7 years ago

Ok, well if I've got you thinking I'd like to add a suggestion: don't try to tackle deserialization. Keep the raw query string in the state (this is enough that the location object is different for every querystring update) and then provide an optional connectRoutes option for deserialization, like toPath and fromPath.

Something like

import 'qs' from querystring
// ...
const { middleware, enhancer, reducer } = connectRoutes(history, routes, { queryParser: qs.parse })

faceyspacey commented 7 years ago

So payload receives no key/values from query strong?

kyeotic commented 7 years ago

I think pushing that responsibility onto the caller is better, it lets each app handle their own format. Though you could also allow connectRoutes to take a querySerializer so that it could handle an object.

Either way, the library's responsibility is very small: take either the literal querystring from the payload, or call the provided serializer with the object, and update the URL.

eudaimos commented 7 years ago

@faceyspacey to follow up on this, filtering and sorting data are necessary to have querystring support.

It can't be done in the path b/c then you are trying to figure out what order to put them in. Also, it is technically the same view of the app and not a different resource you are navigating to when changing how the data is viewed. This also affects how Googlebot will view your site. Google is forgiving of a single page having different views, but would require canonical links for any pages that duplicate content of another page, lest they knock you down for content spamming.

Basically any part of the URL that communicates/embeds state of the app where the ordering of the path cannot be fixed to a logical hierarchy should be part of the querystring. This makes the state of the app at that point both bookmarkable and sharable. This becomes even more necessary for apps that send email notifications.

faceyspacey commented 7 years ago

Yup, @tyrsius already convinced me. Now it's just about adding it to the codebase in the best way possible. Feel free to take peeks at the code and make suggestions or full on submit a PR. I'm working on getting pre-fetching out the gate first. Wait for that to release before I get to this.

elisechant commented 7 years ago

I think it should belong to payload as in location.payload.query as a key value hash object. Would everyone agree? cc @faceyspacey @tyrsius

faceyspacey commented 7 years ago

@elisechant i was thinking location.query and action.meta.location.query to keep payload just to what you explicitly specify. Are there any negative consequences to that?

This hasn't fully seeped in for me yet (i.e. a complete answer hasn't clicked for me yet), but do we want it to function exactly like a regular dispatch. For example: action.type === 'SEARCH' will keep firing every time you change your query parameters, or do we want it to have another type, such as action.type === 'QUERY_UPDATE'. I'm leaning toward the former, as that's kinda what this router is all about. The thought though is this: are there any negative consequences to constantly dispatching the same type when the UI doesn't change much? I guess not--that's the point of Redux. But the point is: it's not a typical state change--so do we represent it differently in any way?

I'm thinking we set location.kind === 'query' or something like this. Or perhaps just query_update. Do we express to the user/developer that the user is just changing search params. Is that information useful?

If this is the primary use-case, let's get it really really right. That's the thinking. So before that, are there other use-cases?

I'm likely overthinking this, but i just want to make sure all needs are checked off. Basically, here's the implementation that will have to change: the system is designed to not dispatch actions corresponding to duplicate URLs (for perf, and consistency). So currently a query string change would not get dispatched. I'd have to change the comparison check to also include the query string. We could look out and it's as simple as that--and I hope that's the case.

As for serialization/deserialization, ultimately it's the easiest part (given it's just a dependency and 2 calls). The only concern is included the bytes of an extra library. What I'm thinking of is this: if you provide serialization/deserialization options, the system will start pumping out a query string key/val in actions and state. If you don't, we don't even bother. I'm also thinking of not having a search string. Is there any real reason to do that. The reason I'm thinking this is the same thinking behind why your actions are true Flux Standard Actions. I.e. they are just type, payload and meta, with our stuff deeply nested in meta. One of the reasons this is important is so tools like redux-actions can make use of it. That said, React Navigation supports breaks away from FSA's a bit since React Navigation does so itself. So anyway, the point is: no more information than is absolutely needed. Developers that don't understand the purpose of search and query strings don't even like to see that stuff. They may fall into the trap of using it when they don't need to. It can be confusing look at a lot of location information. You don't know what's important. What's important in RFR is primarily type and payload of your current and prev route. It was meant to stay that way until this Issue came around. So can we just have a query key or do we need to provide the original search string? Keep in mind you'd be providing your own serialization functions, so no matter what you'd actually get access to the string--it just wouldn't be displayed that way in state or actions. And if you're not providing those functions, it wont show at all.

I know I'm being OCD, but not really. We want to steer users to using this as if it's a Redux app on React Native before the stage where you start to care about URLs. That's the best developer experience for React/Redux imo. The less they are bothered by URL-related concepts the better. There's different skill levels of developers out there, and keeping API surface to a minimum is crucial.

elisechant commented 7 years ago

hmmm. good points.

for me it looks like this:

onClick: () => dispatch({ type: 'USER', payload: { id: 5, search: {'sort':'asc', 'gtm_type':'social'} } })

then:

location.payload.search

faceyspacey commented 7 years ago

...ah i see what you mean. you want to dispatch new query params in the payload. i was thinking about how we store it in the location reducer primarily. But also in action.meta.location.query. So perhaps you can dispatch like that, but we move it to the relevant place. Hmm.

I wonder if it makes sense to break from Flux Standard Actions for this specific thing, i.e. as its own key. Basically search or query now becomes a reserved key name.

If Flux Standard Actions don't matter anymore (as React Navigation determined), it doesn't need to be on the payload. So that's something we need to find out.

elisechant commented 7 years ago

yes you are right. it would also need to work for deep link as well as dynamic routing

faceyspacey commented 7 years ago

by deep link, i assume u mean incoming links. yea, i think the idea that this only works when changing routes is incorrect. it just always works.

but what do u mean by "dynamic routing"? .../pages/:page? ..just to confirm.

elisechant commented 7 years ago

as in client side routing

faceyspacey commented 7 years ago

Guys there's a major update to the demo with SSR + Code Splitting + Authentication/Filtering/Redirecting, etc.

https://github.com/faceyspacey/redux-first-router-demo

Make sure to update your deps in ur apps.

QueryString coming soon. Hopefully next week.

KaRkY commented 7 years ago

I hate to ask but what is the progress on this issue?

faceyspacey commented 7 years ago

It's all good. I'm hoping to get to it next week.

eudaimos commented 7 years ago

@faceyspacey will there be breaking changes from the update adding query string support?

faceyspacey commented 7 years ago

no

faceyspacey commented 7 years ago

Ok, query string support is in the @next dist-tag. Feel free to try it out by doing:

yarn upgrade redux-first-router@next redux-first-router-link@next.

Or if you want to see in action with one click, go to:

https://codesandbox.io/s/pgp5mEkzm?module=r15MmSUmB-

notice the query strings in the links, or how actions now use a query key. Also notice how NavLink's activeClassName feature can't be used with query strings--that's a limitation react-router also has that we have both inherited from path-to-regexp which only handles paths, and is fine given the nature of nav links, and which is also extremely easy to solve via Redux, as you'll see in this sandbox.

To use it, just dispatch your question with a query object at action.query or action.meta.query. The latter is obviously for setups that require true FSAs. I will be doing the former in my apps, and suggest you do the same if you can.

The action after it's gone through the middleware will have a query in both places, regardless of where you put it. If someone requests it in the future, we can add a flag to the options so that it doesn't also appear in action.query if even having it breaks some FSA tool. But, as I recall, from when I used redux-actions, it won't break it, but you won't get access to additional keys on action.

Otherwise, you also need to provide a querySerializer as an option, as seen here:

https://codesandbox.io/s/pgp5mEkzm?module=H1Ebz7rL7rZ

You can assign it the default export of the popular query-string package and done.

I'm going to close this issue. Please open up a new one for any specific issues you come across.

faceyspacey commented 7 years ago

By the way, this has been promoted to the latest dist-tag. The docs for it now live here:

https://github.com/faceyspacey/redux-first-router/blob/master/docs/query-strings.md

Also, there's some updated docs for server-rendering that better reflect the demo:

https://github.com/faceyspacey/redux-first-router/blob/master/docs/server-rendering.md

If you run into any issues, hit me in the Reactlandia gitter chat:

https://gitter.im/Reactlandia/Lobby

UberMouse commented 7 years ago

Hey,

Thanks for adding this, just started converting our app to it yesterday. Now that I've finally got it building (we use typescript so I had to do the entire conversion from our previous routing library in one go before the bundle would rebuild) I've run into a problem with how the query string is put into the store when there isn't one.

Our previous library (redux-little-router) always had a query key on the location reducer, regardless of whether there was a query string in the URL. This made our selectors that extracted query params out of the store easier as all they do is location.query.queryName and they return undefined if the query string isn't present in the URL. With the way query string support was added this causes a bunch of our selectors to crash because location.query is undefined.

Do you think it makes sense to make the query key always present and default to {} if there is no query string in the URL?

faceyspacey commented 7 years ago

for your use case it definitely does. and it's something i debated, and am not opposed to supporting.

here's why i chose not to:

most of the time queries aren't used
i wanted to keep actions and reducers as clean as possible to minimize the cognitive overhead (specifically when looking at the Redux Devtools which can get overwhelming to analyze)

So that was the thinking.

Obviously the fix in userland is an easy check, but i understand it sucks how it makes otherwise elegant code sometimes twice as heavy as the main usage.

I'm somewhat torn on whether to make the change ur requesting, but im in fact leaning towards your side. A PR would make the decision far easier and I'd be likely to accept it quickly.

UberMouse commented 7 years ago

I think that's reasonable. I've actually come up with a simple fix on my end in our selectors to handle the RFR state behavior so I've got that working now.

faceyspacey / redux-first-router

querystrings in state #17