Add ability to share urls. #107

yansern · 2023-09-14T18:35:46Z

Update: I didn't realize there was another PR working on a similar feature here until I submitted this PR. #106

Update 2: I looked into the other PR and I would say the approach taken in this PR is more feature complete. This PR supports restoring the endpoint selected from suggestion dropdown, and the path values of those selected endpoint and also the query params from filled from the UI.

Description

This PR generates a shareable URL and pushes the browser history state when the "Run Endpoint" button is clicked, allowing the URL to be copied from the address bar and shared to colleagues.

In order to achieve this, I created a redux middleware called router that would encode and decode relevant states to and from URLs.

Test Instructions

You can test this with the following urls and you can see the values prefilled:
- Manual URL with different API & Version chosen: http://localhost:3000/?method=GET&url=%252Fsites%252F201582167%252Fsubscribers&api=WP%2520REST%2520API&version=wpcom%252Fv2
- Selected from endpoint suggestion with path values & query params filled: http://localhost:3000/?endpoint=%252Fsites%252F%2524site%252Fcomments%252F%2524comment_ID&method=GET&pathValues=%257B%2522%2524site%2522%253A%2522201582167%2522%257D&queryParams=%257B%2522http_envelope%2522%253A%25220%2522%252C%2522fields%2522%253A%2522ID%252Cpost%252CURL%252Cshort_URL%252Ccontent%2522%252C%2522meta%2522%253A%2522post%252Clikes%2522%252C%2522pretty%2522%253A%2522false%2522%257D&api=WP.COM%2520API&version=v1.1
  - This will take a couple of seconds to load (as it needs to load the endpoints directory first). So it will look like nothing is filled for a few seconds until you see it filled on the screen.
Test entering the URLs signed-out, and then sign-in. After signing-in, you should be redirected back to the same url with all the params intact.

Additional notes

The endpoint doesn't automatically run on page load. It only prefills everything for you. User is expected to click on the "Run Endpoint" button themselves. :)
Pressing "Back" currently does not restore previous state as I foresaw additional complexity around this when trying to implement this and I thought its best left for another PR. User can just refresh the page for now.

Screenshots

TimBroddin

It works, doesn't break anything, and adds great functionality. LGTM!

valterlorran · 2023-09-22T12:17:35Z

I have yet to test it, but it looks great! It's way simpler than my PR. 👍

yansern · 2023-09-23T03:15:45Z

@TimBroddin discovered that the existing implementation of state persistence needsStoring = nextState === state; is most likely incorrect, as it should be needsStoring = nextState !== state;.

At the same time, I'd like to ensure that loading initial state from param also takes in considering of existing data in local storage, which may already have endpoints data loaded, which will shave off a couple of seconds on repeat pageload.

Will continue refining this PR. We're in no rush.

dmsnell

@yansern this is a great patch. I left a lot of comments, but the ones that I think are important are questions about deep coupling between app components. I think we can make a few adjustments and avoid some headaches down the line. thanks for being patient with me!

dmsnell · 2023-09-23T02:26:20Z

.nvmrc

@@ -0,0 +1 @@
+16.20.2


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

dmsnell · 2023-09-23T02:27:59Z

src/api/index.js

@@ -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,


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?

dmsnell · 2023-09-23T02:32:57Z

src/state/index.js

 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 ),


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

dmsnell · 2023-09-23T03:02:07Z

src/state/router.js

+	};
+
+	// Discard empty params
+	Object.keys( params ).forEach( key => ! params[ key ] && ( delete params[ key ] ) );


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

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.

dmsnell · 2023-09-23T03:03:09Z

src/state/router.js

+
+function decodeString( val ) {
+	return decodeURIComponent( val );
+}


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.

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.

dmsnell · 2023-09-23T03:11:10Z

src/state/router.js

+		return false;
+	}
+
+	const state = { request: { ...defaultRequestState }, ui: { ...defaultUiState } };


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.

dmsnell · 2023-09-23T03:12:07Z

src/state/router.js

+		// Fail without breaking the app
+		console.error( 'Could not parse state from url params.', e );
+		return false;
+	}


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?

dmsnell · 2023-09-23T03:15:47Z

src/state/router.js

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


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?

dmsnell · 2023-09-23T03:17:20Z

src/state/router.js

+					}
+					isInitializingEndpoint = false;
+				}
+				break;


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?

dmsnell · 2023-09-23T03:28:53Z

src/state/ui/reducer.js

+	version: null,
+};
+
+const reducer = createReducer( defaultState, {


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, { … } ); }

Added basic routing for shareable urls.

8ede15e

yansern self-assigned this Sep 14, 2023

yansern added the enhancement label Sep 14, 2023

yansern requested review from dmsnell and daledupreez September 14, 2023 18:49

Remove console.log.

8ba207a

TimBroddin approved these changes Sep 21, 2023

View reviewed changes

daledupreez requested a review from valterlorran September 21, 2023 19:04

After signing-in, redirect to the same url you were left before.

fe601db

valterlorran mentioned this pull request Sep 22, 2023

Add the share button #106

Closed

dmsnell reviewed Sep 23, 2023

View reviewed changes

This was referenced Nov 13, 2023

Add Share Button and URL State Setup #116

Closed

Sync state with URL (Share function) #117

Open

		@@ -0,0 +1 @@
		16.20.2

Add ability to share urls. #107

Are you sure you want to change the base?

Add ability to share urls. #107

Conversation

yansern commented Sep 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Test Instructions

Additional notes

Screenshots

Uh oh!

TimBroddin left a comment

Choose a reason for hiding this comment

Uh oh!

valterlorran commented Sep 22, 2023

Uh oh!

yansern commented Sep 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dmsnell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mreishus Nov 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yansern commented Sep 14, 2023 •

edited

Loading

yansern commented Sep 23, 2023 •

edited

Loading

mreishus Nov 13, 2023 •

edited

Loading