State: Type route path & query #55315

chriskmnds · 2021-08-10T12:10:49Z

Changes proposed in this Pull Request

Adds types to the route path and query state actions/reducers/selectors.

I guess more typing can't really hurt. This will also help our work in #55313 with better types coming from the underlying path & query selectors. Hopefully, put some of the groundwork here.

Testing instructions

TBD

Related to #55313, #57109

github-actions · 2021-08-10T12:11:04Z

Link to Calypso live: https://calypso.live?image=registry.a8c.com/calypso/app:build-17844
Link to Jetpack Cloud live: https://calypso.live?image=registry.a8c.com/calypso/app:build-17844&env=jetpack

matticbot · 2021-08-10T12:15:39Z

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

App Entrypoints (~28 bytes added 📈 [gzipped])

name         parsed_size           gzip_size
entry-login        -92 B  (-0.0%)      +24 B  (+0.0%)
entry-main         -63 B  (-0.0%)      +14 B  (+0.0%)

Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used.

Sections (~70 bytes added 📈 [gzipped])

name               parsed_size           gzip_size
home                     +57 B  (+0.0%)      +36 B  (+0.0%)
purchase-product         -35 B  (-0.0%)       +0 B
plans                    -35 B  (-0.0%)      +14 B  (+0.0%)
migrate                  -35 B  (-0.0%)       +1 B  (+0.0%)
media                    -35 B  (-0.0%)      +11 B  (+0.0%)
incoming-redirect        -35 B  (-0.4%)       -8 B  (-0.2%)
accept-invite            -35 B  (-0.0%)      +11 B  (+0.0%)
jetpack-connect          -33 B  (-0.0%)      +16 B  (+0.0%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

Async-loaded Components (~36 bytes added 📈 [gzipped])

name                                               parsed_size           gzip_size
async-load-signup-steps-user                             -35 B  (-0.0%)      +11 B  (+0.0%)
async-load-calypso-post-editor-media-modal               -35 B  (-0.0%)       +9 B  (+0.0%)
async-load-calypso-post-editor-editor-media-modal        -35 B  (-0.0%)       +9 B  (+0.0%)
async-load-calypso-layout-guided-tours                   -35 B  (-0.4%)       +7 B  (+0.2%)
async-load-design-blocks                                 -33 B  (-0.0%)      +17 B  (+0.0%)

React components that are loaded lazily, when a certain part of UI is displayed for the first time.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

chriskmnds · 2021-08-13T08:02:32Z

client/state/route/types.ts

+export type Query = {
+	[ key: string ]: string | number | undefined;
+	_timestamp?: number;
+};


Not fun of this type. Hopefully, there's a way to express it differently. _timestamp is the only property of type number that may also be undefined - everything else is a string.

The _timestamp shouldn't really be here on the query, but rather on a separate property of the state.route object. There would be { path, query, timestamp } properties there.

The _timestamp field is currently used only by the Guided Tours code and nowhere else. The meaning is "the time at which the last (initial, previous) route navigation happened".

Adding types reveals such hacks, like slapping non-query data on the query object, and nudges us toward fixing them.

jsnajdr · 2021-08-20T06:32:30Z

client/state/route/types.ts

+export type Query = {
+	[ key: string ]: string | number | undefined;
+	_timestamp?: number;
+};


The _timestamp shouldn't really be here on the query, but rather on a separate property of the state.route object. There would be { path, query, timestamp } properties there.

The _timestamp field is currently used only by the Guided Tours code and nowhere else. The meaning is "the time at which the last (initial, previous) route navigation happened".

Adding types reveals such hacks, like slapping non-query data on the query object, and nudges us toward fixing them.

jsnajdr · 2021-08-20T06:34:13Z

client/state/route/types.ts

+
+export type RouteSetActionCreator = (
+	path: RouteSetAction[ 'path' ],
+	query: RouteSetAction[ 'query' ]


The types here are Path and Query, there's no need to specify them in a verbose way like this.

jsnajdr · 2021-08-20T06:40:32Z

client/state/route/query/reducer.ts

+const currentReducer = ( state: QueryState[ 'current' ], query: Query ) =>
+	! isEqualQuery( state, query ) ? timestamped( query ) : state;
+
+const initialState = {


const initialState: QueryState = { initial: false, ... }

is better. It avoids the need to write as const.

jsnajdr · 2021-08-20T08:23:36Z

client/state/route/types.ts

+	previous: Query | false;
+};
+
+export type Path = string;


Type aliases like this are not very useful in TypeScript, where the type checking is structural rather than nominal. Simply using string everywhere is OK.

jsnajdr · 2021-08-20T08:25:54Z

client/state/route/types.ts

+
+export type RouteState = {
+	path: PathState;
+	query: QueryState;


There is also a lastNonEditorRoute: string field in RouteState

jsnajdr · 2021-08-20T08:27:09Z

client/state/selectors/get-current-query-arguments.ts

+ */
+export function getCurrentQueryArguments(
+	state: IAppState
+): IAppState[ 'route' ][ 'query' ][ 'current' ] | null {


The return type is Query, so why not use it instead?

jsnajdr · 2021-08-20T08:30:15Z

client/state/route/last-non-editor-route/reducer.ts

+/**
+ * Internal dependencies
+ */
+import { ROUTE_CLEAR_LAST_NON_EDITOR, ROUTE_SET } from 'calypso/state/action-types';


This PR adds a reducer.ts file, but doesn't delete the old reducer.js one.

jsnajdr · 2021-08-20T08:34:24Z

client/state/route/types.ts

+	path: Path;
+	query: Query;
+	type: typeof ROUTE_SET;
+};


I think that diligently declaring strict types for the Route* actions is not necessary. The types are not actually used anywhere.

The setRoute action creator (like any other action creator) returns an opaque action object whose internal structure is not interesting. The action is only dispatched by passing it to dispatch and its fields could as well be private.

It's only the reducer that actually reads them, but our reducer is declared as accepting AnyAction and doesn't use the specific types.

Because of that, the actions creators could simply return the Action type from redux.

getCurrentQueryArguments was used, accessing email_address, without checking if it was null

Removed unused types

jsnajdr · 2021-11-01T10:46:01Z

client/data/home/use-home-layout-query-params.ts

@@ -8,7 +9,8 @@ export interface HomeLayoutQueryParams {
 }

 export function useHomeLayoutQueryParams(): HomeLayoutQueryParams {
-	const { dev, view } = useSelector( getCurrentQueryArguments ) as any;
+	const dev = get( useSelector( getCurrentQueryArguments ), 'dev', undefined );
+	const view = get( useSelector( getCurrentQueryArguments ), 'view', undefined );


This doesn't look like an improvement. Just const { dev, view } = useSelector( ... ) should work. If it doesn't, there's something wrong with the types and they should be fixed. Calling the selector twice and introducing a new usage of Lodash is not good.

jsnajdr · 2021-11-01T10:48:43Z

client/state/selectors/get-current-query-arguments.ts

+ * Kept for backwards compatibility in case any null checks.
+ * Investigate and refactor.
+ */
+export function getCurrentQueryArguments( state: IAppState ): Query | false | null {


The return type should be just Query, and the selector should return an empty {} object if the current URL doesn't have a query string. That's the API most convenient for the caller.

I feel the selector could just return whatever state.route.query.current points to. 🤔 No need to coalesce null in the mix as we do here either (as also done previously). So either undefined or state.route.query.current -> ultimately, just return state?.route?.query?.current;.

This makes sense to me in the following way:

the selector becomes a simple/dumb reflection of the state tree

the default {} (or whatever else we choose) becomes the responsibility of the reducer

undefined for something that has not been defined yet may carry meaning of its own, and a raw/base selector can reflect that

Of course, trade-offs e.g. a derived state selector will ultimately provide some sort of default, which they'd be responsible for defining anyhow. So the alternative (what you mention I think) might just be better e.g. selectors always defining and returning the default values.

Yes, I think the false value for the state doesn't need to be there, in neither of initial, current and previous. It can be an empty object instead:

const EMPTY_QUERY = {}; const initialState = { initial: EMPTY_QUERY, current: EMPTY_QUERY, previous: EMPTY_QUERY, };

And the reducer for initial can do a check like:

initial = state.initial === EMPTY_QUERY ? action.query : state.initial

to set the initial state only on the very first ROUTE_SET. Even if action.query will be an empty object, it will be a different empty object than the initial state.

That's perfect. Thanks! We'll take this path.

chriskmnds added [Type] Enhancement [Status] In Progress State labels Aug 10, 2021

chriskmnds self-assigned this Aug 10, 2021

chriskmnds added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Aug 12, 2021

chriskmnds mentioned this pull request Aug 12, 2021

State: Selector to get current route path and query #55313

Open

chriskmnds requested review from razvanpapadopol and jsnajdr August 13, 2021 07:37

chriskmnds commented Aug 13, 2021

View reviewed changes

jsnajdr reviewed Aug 20, 2021

View reviewed changes

chriskmnds assigned escapemanuele Oct 18, 2021

chriskmnds removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Oct 25, 2021

Type current path & query selectors

9faa750

chriskmnds force-pushed the add/state-route-types branch from 2745aab to 9faa750 Compare October 27, 2021 13:03

matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Oct 27, 2021

Emanuele Buccelli added 2 commits October 28, 2021 15:16

log-in/new without query parameters threw error

cddfcde

getCurrentQueryArguments was used, accessing email_address, without checking if it was null

Fixed error in useHomeLayoutQueryParams

27f1e55

Removed unused types

jsnajdr reviewed Nov 1, 2021

View reviewed changes

jsnajdr mentioned this pull request Nov 1, 2021

Redux route state: store timestamp outside query #57500

Merged

escapemanuele mentioned this pull request Nov 3, 2021

State: Route query initial state is null #57610

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

State: Type route path & query #55315

State: Type route path & query #55315

chriskmnds commented Aug 10, 2021 •

edited

Loading

github-actions bot commented Aug 10, 2021 •

edited by matticbot

Loading

matticbot commented Aug 10, 2021 •

edited

Loading

chriskmnds Aug 13, 2021

jsnajdr Aug 20, 2021

jsnajdr Aug 20, 2021

jsnajdr Aug 20, 2021

jsnajdr Aug 20, 2021

jsnajdr Aug 20, 2021

jsnajdr Aug 20, 2021

jsnajdr Aug 20, 2021

jsnajdr Aug 20, 2021

jsnajdr Aug 20, 2021

jsnajdr Nov 1, 2021

jsnajdr Nov 1, 2021

chriskmnds Nov 1, 2021 •

edited

Loading

jsnajdr Nov 1, 2021 •

edited

Loading

chriskmnds Nov 1, 2021

State: Type route path & query #55315

Are you sure you want to change the base?

State: Type route path & query #55315

Conversation

chriskmnds commented Aug 10, 2021 • edited Loading

Changes proposed in this Pull Request

Testing instructions

github-actions bot commented Aug 10, 2021 • edited by matticbot Loading

matticbot commented Aug 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriskmnds Nov 1, 2021 • edited Loading

Choose a reason for hiding this comment

jsnajdr Nov 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriskmnds commented Aug 10, 2021 •

edited

Loading

github-actions bot commented Aug 10, 2021 •

edited by matticbot

Loading

matticbot commented Aug 10, 2021 •

edited

Loading

chriskmnds Nov 1, 2021 •

edited

Loading

jsnajdr Nov 1, 2021 •

edited

Loading