Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(preact-iso): Use pushState/ replaceState for useLocation().route method #413

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

(preact-iso): Use pushState/ replaceState for useLocation().route method #413

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
56f83c8
Use history.replaceState for route function
piotr-cz Mar 9, 2021
43214d3
Add semicolons
piotr-cz Mar 9, 2021
0faacea
Add option to use object as a path
piotr-cz Mar 9, 2021
25fd059
Change object signature
piotr-cz Mar 9, 2021
7337f1c
Merge branch 'main' into feature/preact-iso-route-location
piotr-cz Mar 12, 2021
1f5654b
Merge branch 'main' into feature/preact-iso-route-location
piotr-cz Mar 15, 2021
d772def
Update code as per https://github.com/preactjs/wmr/pull/413#discussio…
piotr-cz Mar 15, 2021
81f18f7
Wrap reducer to allow different signature
piotr-cz Mar 22, 2021
3d0d9f4
Add changeset
piotr-cz Mar 24, 2021
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
2 changes: 1 addition & 1 deletion packages/preact-iso/router.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ interface LocationHook {
url: string;
path: string;
query: Record<string, string>;
route: (url: string) => void;
route: (url: string | { url: string, replace?: boolean }) => void;
Copy link
Member

Choose a reason for hiding this comment

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

Wondering if we can use (url, replace) here? That would match preact-router.

};
export const useLocation: () => LocationHook;

Expand Down
24 changes: 21 additions & 3 deletions packages/preact-iso/router.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,34 @@
import { h, createContext, cloneElement } from 'preact';
import { useContext, useMemo, useReducer, useEffect, useLayoutEffect, useRef } from 'preact/hooks';

const UPDATE = (state, url, push) => {
if (url && url.type === 'click') {
/**
* @param {string} state
* @param {MouseEvent|PopStateEvent|Object|string} url
* @return {string}
*/
const UPDATE = (state, url) => {
/** @type {boolean|undefined} - History state update strategy */
let push = undefined

// user click
if (url instanceof MouseEvent) {
piotr-cz marked this conversation as resolved.
Show resolved Hide resolved
// @ts-ignore-next
const link = url.target.closest('a[href]');
if (!link || link.origin != location.origin) return state;

url.preventDefault();
push = true;
url = link.href.replace(location.origin, '');
} else if (typeof url !== 'string') {
// navigation
} else if (url instanceof PopStateEvent) {
url = location.pathname + location.search;
// manual invocation: route({ path, replace })
} else if (typeof url === 'object') {
Copy link
Member

Choose a reason for hiding this comment

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

Here's what I'd suggest, in order to preserve the duck typing (as mentioned in previous comment):

const UPDATE = (state, update) => {
	/** @type {boolean|undefined} - History state update strategy */
	let push, url;

	if (!update || typeof update === 'string') {
		url = update;
		push = true;
	}
	else if (update.type === 'click') {
		// user click
		// @ts-ignore-next
		const link = update.target.closest('a[href]');
		if (!link || link.origin != location.origin) return state;

		update.preventDefault();
		url = link.pathname + link.search + link.hash;
		push = true;
	}
	else if (update.type === 'popstate') {
		// navigation
		url = location.pathname + location.search + location.hash;
	}
	else {
		// manual invocation: route(url) or route({ url, replace })
		url = update.url || update;
		push = !url.replace;
	}

	if (push === true) history.pushState(null, '', url);
	else if (push === false) history.replaceState(null, '', url);
	return url;
}

Copy link
Contributor Author

@piotr-cz piotr-cz Mar 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Should we allow passing a value that evaluates to false (undefined/ null) with route method?
If so, then should it be added to history?

	if (!update || typeof update === 'string') {
	//  ^

When the update is not either string or event I'd say it must be an object?

	else {
		// manual invocation: route(url) or route({ url, replace })
		url = update.url || update;
		//                  ^

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated PR with code you suggested, adapting to eslint/ prettier rules

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@developit
What do you think about my comments above?

url = url.url;
push = !url.replace;
// manual invocation: route(path)
} else {
push = true;
}

if (push === true) history.pushState(null, '', url);
Expand Down