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.

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

Conversation

piotr-cz
Copy link
Contributor

@piotr-cz piotr-cz commented Mar 9, 2021

At this moment when using useLocation().route('/foobar') window location doesn't change: neither history.pushState nor history.replaceState method is used.

This pull request

  1. changes method behavior in that window location will be added to history via pushState.
  2. allows using object as a parameter so it's possible to use replaceState
// Use history.pushState
route('/foobar);
route({ url: '/foobar' });
route({ url: '/foobar', replace: false });

// Use history.replaceState
route({ url: '/foobar', replace: true });

Originally the useReducer hook dispatcher had a third push argument, but dispatchers consume only two arguments.

const UPDATE = (state, url, push) => {

I'm not sure if I should bump patch or minor version with changeset?

@changeset-bot
Copy link

changeset-bot bot commented Mar 9, 2021

🦋 Changeset detected

Latest commit: 3d0d9f4

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
preact-iso Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@piotr-cz piotr-cz changed the title (preact-iso): Use history.replaceState for route function (preact-iso): Use pushState/ replaceState for useLocation().route method Mar 9, 2021
Copy link
Member

@developit developit left a comment

Choose a reason for hiding this comment

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

It looks like this was already supported?

route('/url', true)

(admittedly, push=true might be a better default)

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

@piotr-cz
Copy link
Contributor Author

piotr-cz commented Mar 9, 2021

@developit
Unfortunately passing replace flag as additional argument doesn't work, as route is actually a reducers dispatcher, which accepts only one argument (see docs > useReducer):

const [url, route] = useReducer(UPDATE, location.pathname + location.search);
const value = useMemo(() => {
const u = new URL(url, location.origin);
const path = u.pathname.replace(/(.)\/$/g, '$1');
// @ts-ignore-next
return { url, path, query: Object.fromEntries(u.searchParams), route };
}, [url]);

Probably we would have to refactor router code to not use reducer in this case (which may be not a bad idea to handle history outside/ without reducer).

@piotr-cz
Copy link
Contributor Author

piotr-cz commented Mar 11, 2021

I prefer route(url, replace) as well, but as long as there is no better solution, let's keep this PR opened for reference

@piotr-cz
Copy link
Contributor Author

I've created new PR in #424 to fix the bug with location change after using useLocation().route(path).

After it's resolved it will be easier to move forward with adding an option such as route(path, replace) either with this PR new one.

@developit
Copy link
Member

Nice - thanks for pulling that out, just merged it.

packages/preact-iso/router.js Outdated Show resolved Hide resolved
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

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?

@piotr-cz
Copy link
Contributor Author

I've updated PR to use code suggested in #413 (comment)

However I'm not happy with the route({ url, replace }) signature (proposed by me) and would prefer route(url, replace) instead.
Let's not merge this PR yet, we can do better.

@developit
Copy link
Member

developit commented Mar 19, 2021

@piotr-cz I think we might be able to create a wrapper function in the provider that swaps the argument signature:

-	const [url, route] = useReducer(UPDATE, location.pathname + location.search);
+	const [url, doRoute] = useReducer(UPDATE, location.pathname + location.search);
+	const route = useCallback((url, replace) => doRoute({ url, replace }), []);

It might even be possible to simplify the history event handling this way too.

@piotr-cz
Copy link
Contributor Author

@developit
Updated, thank you for suggestion!

@piotr-cz
Copy link
Contributor Author

Just one question:
Should I add changeset with patch or minor version bump?
As the route method is not documented, I guess patch is enough.

@JoviDeCroock
Copy link
Member

I'd go for patch yea

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants