Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .nvmrc
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
16.20.2
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

was this inclusion intentional? the package.json already specifies node@>=16.0.0 but this one is more specific, and doesn't reference npm

2 changes: 1 addition & 1 deletion src/api/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ if ( wpcomConfig ) {
id: 'WPCOM',
baseUrl: 'https://public-api.wordpress.com/oauth2',
userUrl: 'https://public-api.wordpress.com/rest/v1.1/me',
redirectUrl: wpcomConfig.redirectUrl || wpcomConfig.redirect_uri,
redirectUrl: window.location.href,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

is this the proper change here? if someone uses the current location then it seems like it could lead to oauth failures, ass each oauth client application is setup for a specific redirect url. can you share some context on the change?

clientId: wpcomConfig.clientID || wpcomConfig.clientId || wpcomConfig.client_id,
scope: 'global',
};
Expand Down
3 changes: 1 addition & 2 deletions src/config.sample.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
{
"wordpress.com": {
"client_id": "12345",
"redirect_uri": "http://example.com/path/to/app"
"client_id": "12345"
}
}
8 changes: 7 additions & 1 deletion src/lib/redux/cache.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,13 @@ function deserialize( state, reducer ) {
return reducer( state, { type: DESERIALIZE } );
}

export function loadInitialState( initialState, reducer ) {
export function loadInitialState( initialState, initialStateFromUrl, reducer ) {

// URL state overrides local storage state
if ( initialStateFromUrl ) {
return reducer( initialStateFromUrl, { type: DESERIALIZE } );
}

const localStorageState = JSON.parse( localStorage.getItem( STORAGE_KEY ) ) || {};
if ( localStorageState._timestamp && localStorageState._timestamp + MAX_AGE < Date.now() ) {
return initialState;
Expand Down
6 changes: 3 additions & 3 deletions src/state/index.js
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
import { createStore, applyMiddleware } from 'redux';
import thunk from 'redux-thunk';

import router, { getStateFromUrl } from './router';
import reducer from './reducer';
import { boot } from './security/actions';
import { loadInitialState, persistState } from '../lib/redux/cache';

const store = createStore(
reducer,
loadInitialState( {}, reducer ),
applyMiddleware( thunk )
loadInitialState( {}, getStateFromUrl(), reducer ),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this is a nice setup for passing this information; I appreciate the thought that went into your design

applyMiddleware( thunk, router )
);
persistState( store, reducer );
store.dispatch( boot() );
Expand Down
2 changes: 1 addition & 1 deletion src/state/request/reducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import {
} from '../actions';
import schema from './schema';

const defaultState = {
export const defaultState = {
method: 'GET',
endpoint: false,
pathValues: {},
Expand Down
142 changes: 142 additions & 0 deletions src/state/router.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,142 @@

import { REQUEST_TRIGGER, API_VERSIONS_RECEIVE, REQUEST_SELECT_ENDPOINT } from './actions';
import { defaultState as defaultRequestState } from './request/reducer';
import { defaultState as defaultUiState } from './ui/reducer';
import { loadEndpoints } from './endpoints/actions';
import { getEndpoints } from './endpoints/selectors';

function getUrlParams() {
return new URL( window.location.href ).searchParams;
}

function encodeString( val ) {
return ( val === null || val === undefined ) ? '' : encodeURIComponent( val );
}

function encodeObject( val ) {
return ( typeof val === 'object' ) && Object.keys( val ).length === 0 ? '' : encodeURIComponent( JSON.stringify( val ) );
}

function decodeString( val ) {
return decodeURIComponent( val );
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

is this offering much over directly calling decodeURIComponent()? I find that the abstraction hasn't made the code clearer to me, and from the way it's used, it seems like it's only being used in the context of a query string where decodeURIComponent is a fitting match.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It seems that it's making a pair between encodeString and decodeString, it's clear that those do the inverse of each other. encodeObject and decodeObject is a complementary pair. Replacing one of those 4 with a direct call makes it more confusing imo.


function decodeObject( val ) {
return JSON.parse( decodeURIComponent( val ) );
}

export const getUrlFromState = state => {

const { request, ui } = state;

const params = {
bodyParams: encodeObject( request.bodyParams ),
endpoint: encodeString( request.endpoint ? request.endpoint.pathLabeled : '' ),
method: encodeString( request.method ),
pathValues: encodeObject( request.pathValues ),
queryParams: encodeObject( request.queryParams ),
url: encodeString( request.url ),
api: encodeString( ui.api ),
version: encodeString( ui.version ),
};

// Discard empty params
Object.keys( params ).forEach( key => ! params[ key ] && ( delete params[ key ] ) );
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

there's a value in this terseness, but we don't often use the pattern of ** for control flow, and given how infrequently this project is maintained, it might be more valuable to choose something more familiar, including constructing the object positively instead of removing empty bits.

const urlParams = new URLSearchParams();

for ( const [ value, key ] of Object.entries( params ) ) {
	if ( value ) {
		urlParams.update( key, value );
	}
}

also is the basic truthiness check valid here for all the parameters? it seems like we might end up needlessly encoding empty objects and omit false values.

I wonder if it would help to use undefined to include these when forming the params object. url: '' === request.url ? encodeString( request.url ) : undefined

Copy link
Copy Markdown
Contributor

@mreishus mreishus Nov 13, 2023

Choose a reason for hiding this comment

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

Nit: const [ value, key ] of Object.entries( params ) should be const [ key, value ] of Object.entries( params ), or weird things start happening. Also .update -> .set.


// Construct url
const urlParams = new URLSearchParams( params );
const url = window.location.origin + window.location.pathname + '?' + urlParams.toString();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we can create a new URL( window.location ) here and then directly use our search params to replace any existing ones.

const url = new URL( window.location );
url.search = urlParams;

return url;


return url;
};

export const getStateFromUrl = () => {
const urlParams = getUrlParams();

if ( urlParams.toString() === '' ) {
return false;
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

there's a size property we can examine.

if ( urlParams.size === 0 ) {
	return false;
}


const state = { request: { ...defaultRequestState }, ui: { ...defaultUiState } };
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

it would be best to avoid such deep coupling outside of the react tree. I can easily imagine someone adding another top-level state property and not realizing this code needs it.

I'm not sure we even need to return the full state object here as it seems like we could treat this as state augmentation. we can pass this into the initial state tree and merge it with the defaults there. that way these two parts of the system can operate independently.


try {
for ( const [ key, value ] of urlParams.entries() ) {
switch ( key ) {
case 'bodyParams':
state.request.bodyParams = decodeObject( value );
break;
case 'method':
state.request.method = decodeString( value );
break;
case 'pathValues':
state.request.pathValues = decodeObject( value );
break;
case 'queryParams':
state.request.queryParams = decodeObject( value );
break;
case 'url':
state.request.url = decodeString( value );
break;
case 'api':
state.ui.api = decodeString( value );
break;
case 'version':
state.ui.version = decodeString( value );
break;
default:
break;
}
}
} catch ( e ) {
// Fail without breaking the app
console.error( 'Could not parse state from url params.', e );
return false;
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

it's a choice to bail if any parameter doesn't decode, but would we want to try and pre-fill whatever parameters we can decode?


return state;
};

const router = ( { getState, dispatch } ) => {

// Determine if we need to load and select the endpoint
let isInitializingEndpoint = false;

const { ui = {} } = getStateFromUrl();
const endpointUrlParam = getUrlParams().get( 'endpoint' );
const endpointPathLabeled = endpointUrlParam ? decodeURIComponent( endpointUrlParam ) : false;
const apiName = ui.api;
const apiVersion = ui.version;

if ( endpointPathLabeled && apiName && apiVersion ) {
loadEndpoints( apiName, apiVersion )( dispatch );
isInitializingEndpoint = true;
}

return next => action => {
const state = getState();

switch ( action.type ) {
case REQUEST_TRIGGER:
const url = getUrlFromState( state );
window.history.pushState( {}, document.title, url );
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

MDN suggests that the middle parameter is unused, so passing a real value could be confusing since it won't have an impact in the browser. should we pass '' here instead?

break;
case API_VERSIONS_RECEIVE:
// Once the endpoint is loaded, select the endpoint.
if ( isInitializingEndpoint ) {
const endpoints = getEndpoints( state, apiName, apiVersion );
const endpoint = endpoints.find( ( { pathLabeled } ) => pathLabeled === endpointPathLabeled );
if ( endpoint ) {
dispatch( { type: REQUEST_SELECT_ENDPOINT, payload: { endpoint } } );
}
isInitializingEndpoint = false;
}
break;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nice queueing of the initial request.
do we need to make allotments for logged-out views here or will this only run if someone is logged in?

default:
break;
}

return next( action );
};
};

export default router;
7 changes: 6 additions & 1 deletion src/state/ui/reducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,12 @@ import { UI_SELECT_API, UI_SELECT_VERSION } from '../actions';
import { getDefault } from '../../api';
import schema from './schema';

const reducer = createReducer( { api: getDefault().name, version: null }, {
export const defaultState = {
api: getDefault().name,
version: null,
};

const reducer = createReducer( defaultState, {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

given that the reducer ultimately governs its own state and structure, I would feel better if this code merged the initial values from cache and from the URL, and then we could leave both of those systems decoupled from each other. this would require making this export something like makeReducer that takes both of those and merges them.

export const makeReducer = ( { persistedState = null, urlState = null } ) => {
	const initialState = Object.assign(
		{},
		defaultState,
		persistedState,
	);

	if ( urlState ) {
		initialState.request.bodyParams = urlState.bodyParams;
		initialState.request.method = urlState.method;
		
	}

	return createReducer( initialState, { … } );
}

[ UI_SELECT_API ]: ( state, { payload } ) => {
return ( {
version: null,
Expand Down