Rework query parsing #2396
Rework query parsing #2396
Conversation
Adds an IRoutePropsWithFetch definition for routes with getQueryParams or fetchInitialData to cause compiler errors when the types no longer match.
Problem: A search for "%ab" produces a url with "%25ab". Refreshing the page results in URLSearchParams turning "%25ab" back into "%ab". decodeURIComponent() then complains about "%ab" being malformed. This removes decodeURIComponent() calls for query parameters and composes all query strings with getQueryString(), which now uses URLSearchParams. Query parsing already goes through getQueryParams() which also uses URLSearchParams.
| const communityForm: GetCommunity = { | ||
| name: communityName, | ||
| }; | ||
|
|
||
| const dataType = getDataTypeFromQuery(urlDataType); |
There was a problem hiding this comment.
Getting rid of these is much cleaner, thx.
I tested it with a downgraded version of - "lemmy-js-client": "0.19.4-alpha.4",
+ "lemmy-js-client": "0.19.4-alpha.2",This downgrades cross-fetch. Without this I get |
|
We removed the dependency on cross-fetch in this one. I created a deploy with that included, could you try this version out?
|
src/shared/components/app/app.tsx
Outdated
| let queryProps = routeProps; | ||
| if (getQueryParams) { | ||
| if (this.isoData.site_res) { | ||
| queryProps = { | ||
| ...queryProps, | ||
| ...getQueryParams( | ||
| routeProps.location.search, | ||
| this.isoData.site_res, | ||
| ), | ||
| }; | ||
| } else { | ||
| // ErrorGuard will catch this | ||
| } | ||
| } |
There was a problem hiding this comment.
If the else block is going to be empty, why nest the ifs instead of just checking getQueryParams && this.isoData.site_res?
There was a problem hiding this comment.
This was intended to point out that a RouteComponent with getQueryParams will never see props without that don't contain the query parameters.
| FallbacksT extends Empty = Empty, | ||
| >( | ||
| processors: QueryMapping<PropsT, FallbacksT>, | ||
| source: string | undefined, |
There was a problem hiding this comment.
Will source?; string, work here?
| >( | ||
| processors: QueryMapping<PropsT, FallbacksT>, | ||
| source: string | undefined, | ||
| fallbacks: FallbacksT, |
There was a problem hiding this comment.
A default value would be good here. I noticed that you pass an empty object as the third arg to this function practically everywhere you call it.
| fallbacks: FallbacksT, | |
| fallbacks: FallbacksT = {}, |
| type Empty = NonNullable<unknown>; | ||
|
|
||
| type QueryMapping<PropsT, FallbacksT extends Empty> = { | ||
| [K in keyof PropsT]-?: ( |
There was a problem hiding this comment.
Why the - before the ?? I've never seen that syntax used in typescript before.
There was a problem hiding this comment.
Makes specifying a mapping for each property manadatory. Works like Required<T>:
/**
* Make all properties in T required
*/
type Required<T> = {
[P in keyof T]-?: T[P];
};
|
|
|
Yep, that new lemmy-js-client dep does work correctly, could you also include that too. |
Is already part of the Small getQueryParams cleanup commit |
Description
Passes the result of the
getQueryParamsimplemention for a component, to the component aspropsand to itsfetchInitialDataasquery. This makes server and client use the same methods to parse query parameters. Also passes the result ofmatchPathasmatchtofetchInitialData, this is the same asprops.match.As a result the server side rendering matches a lot closer the client side rendering. Previously the server only used the query parameters to fetch data, but ignored them during
renderToString.Fixes an issue with double decoding query parameters, where the result of the first decoding can create malformed tokens for the second decoding. The fix relies on always using
URLSearchParamsto parse and create query strings.Fixes
<PictrsImage />with an image like"../pictrs/image/...webp?some=thing"trying to show thumbnails ending in"?some=thing?thumbnail..".