feat: expose hash from `useRouter` #746

pmelab · 2024-06-13T11:30:13Z

What was changed?

Expose the current url hash via UNSTABLE_useRouter. On the server, its always an empty string, but gets populated on the client after the initial render.

Why was it changed?

Otherwise inconvenient useEffect-gymnastics are necessary to get the current hash reliably into a client component.

Notes

In the process of writing tests I found another potential issue with searchParams.

https://github.com/dai-shi/waku/pull/746/files#diff-a8dab8847d02ba73c6b66e738495556ddda5d2dd01b70322aae006f7669ea237R6-R11

I'm not sure if we should move the double-creation of URLSearchParams into the useRouter hook. Given some advice, I can try to fix that as well.

vercel · 2024-06-13T11:30:17Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
waku	✅ Ready (Inspect)	Visit Preview	Jul 25, 2024 2:48pm

codesandbox-ci · 2024-06-13T11:30:36Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

dai-shi

Thanks for the suggestion!

packages/waku/src/router/common.ts

dai-shi · 2024-06-13T23:44:19Z

e2e/fixtures/use-router/src/TestRouter.tsx

+export default function TestRouter() {
+  const router = useRouter_UNSTABLE();
+  // TODO: Why is this necessary? On the client `searchParams`
+  //       is a URLSearchParams object, but on the server it's an array.


I didn't know it's an array on the server. How can it be??

Not sure, but without that, it breaks with get is not a function. And the debugger showed me an array. Maybe node version differences?

Can you dig into it further? If it's a bug in Waku, we would like to fix it.

Sorry, took me some time to get back to this. I was not able to pinpoint whats the problem. This is what I have found.

At this point, searchParams is still an URLSearchParams object:

waku/packages/waku/src/router/define-router.ts

Lines 217 to 228 in f61b264

const input = getInputString(pathname);

const body = createElement(

ServerRouter as FunctionComponent<

Omit<ComponentProps<typeof ServerRouter>, 'children'>

>,

{ route: { path: pathname, searchParams } },

componentIds.reduceRight(

(acc: ReactNode, id) => createElement(Slot, { id, fallback: acc }, acc),

null,

),

);

return { input, body };

But when ServerRouter gets rendered, it has become []:

waku/packages/waku/src/router/client.ts

Lines 530 to 552 in f61b264

export function ServerRouter({

children,

route,

}: {

children: ReactNode;

route: RouteProps;

}) {

return createElement(

Fragment,

null,

createElement(

RouterContext.Provider,

{

value: {

route,

changeRoute: notAvailableInServer('changeRoute'),

prefetchRoute: notAvailableInServer('prefetchRoute'),

},

},

children,

),

);

}

My guess is that the route definition gets serialized by react when passed into the ServerRouter client component and the empty URLSearchParams map gets turned into []?

With ddb4086 I removed the additional construction of URLSearchParams to see if it also happens on CI, which it does.

@dai-shi : i tried to find out more, but basically thats it. An empty URLSearchParams object gets passed as a prop into createElement and inside the ServerRouter its an empty array. It has to be reacts prop-handling. Maybe it would be safer to stringify searchParams?

Yeah, you are absolutely right.

repro:

$ npm create waku@next $ cd ... $ npm i $ cat foo.cjs const { renderToPipeableStream } = require('react-server-dom-webpack/server'); const data = new URLSearchParams(); renderToPipeableStream(data).pipe(process.stdout); $ node --conditions react-server foo.cjs 0:[]

I will open another PR to make it a string and see how it goes.

pmelab · 2024-06-14T03:48:59Z

To be transparent, this is the code that we save in the consumer:

  const router = useRouter();
  const [hash, setHash] = useState('');
  useEffect(() => {
    setHash(window.location.hash);
  }, [router]);

Its not gigantic, but useEffect has a bad reputation lately.

Co-authored-by: Daishi Kato <dai-shi@users.noreply.github.com>

dai-shi

Sorry for the delay.
It looks good overall. Comments below.

dai-shi · 2024-06-19T00:47:11Z

e2e/fixtures/use-router/src/TestRouter.tsx

+export default function TestRouter() {
+  const router = useRouter_UNSTABLE();
+  // TODO: Why is this necessary? On the client `searchParams`
+  //       is a URLSearchParams object, but on the server it's an array.


Can you dig into it further? If it's a bug in Waku, we would like to fix it.

packages/waku/src/router/client.ts

dai-shi · 2024-06-19T00:50:55Z

packages/waku/src/router/client.ts

+        ...parseRoute(new URL(window.location.href)),
+        // Make sure there is no hash on the initial route,
+        // so it matches the server-rendered version. Otherwise
+        // there might be hydration errors.
+        hash: '',


It looks a bit hacky. Maybe we make parseRoute to accept an option?

Co-authored-by: Daishi Kato <dai-shi@users.noreply.github.com>

Necessary for the initial render, where the hash would cause a hydration mismatch.

#746 revealed that `URLSearchParams` doesn't work. (In the near future, I will also rework on fetchRSC to eliminate it.)

dai-shi · 2024-07-24T00:47:27Z

@pmelab Hi, can you merge main?

dai-shi

2nd review

packages/waku/src/router/client.ts

dai-shi · 2024-07-25T09:18:41Z

packages/waku/src/router/client.ts

@@ -301,8 +305,15 @@ const InnerRouter = ({ routerData }: { routerData: RouterData }) => {
  const refetch = useRefetch();

  const [route, setRoute] = useState(() =>
-    parseRoute(new URL(window.location.href)),
+    parseRoute(new URL(window.location.href), true),


So, ignoreHash is used only here.

Yes, without this, there are hydration errors, because the server has no hash, but the first client render does.

Oh, I see. Can you leave some comments in the code?
I might refactor things around here in the future.

I added some explanations there

packages/waku/src/router/client.ts

Co-authored-by: Daishi Kato <dai-shi@users.noreply.github.com>

dai-shi

3rd review.

dai-shi · 2024-07-25T14:19:57Z

packages/waku/src/router/client.ts

+  // Update the route post-load to include the current hash.
+  useEffect(() => {
+    setRoute(parseRoute(new URL(window.location.href)));
+    // eslint-disable-next-line react-hooks/exhaustive-deps


Hm, I wonder why we need the exception.

dai-shi · 2024-07-25T14:21:32Z

packages/waku/src/router/client.ts

+
+  // Update the route post-load to include the current hash.
+  useEffect(() => {
+    setRoute(parseRoute(new URL(window.location.href)));


This actually triggers re-renders every time, even if the hash is ''. We should probably improve it.

dai-shi · 2024-07-25T14:43:35Z

working on it.

dai-shi

Should be good.

pmelab added 4 commits June 13, 2024 12:21

test: test case for basic useRouter usage

a4965b6

test: extend test cases for hash retrieval

82365c5

feat: extend useRouter to also emit the current hash

a9d41ec

chore: lockfile update

6fc4cac

dai-shi reviewed Jun 13, 2024

View reviewed changes

packages/waku/src/router/common.ts Outdated Show resolved Hide resolved

dai-shi reviewed Jun 13, 2024

View reviewed changes

refactor: make hash mandatory

bde826b

Co-authored-by: Daishi Kato <dai-shi@users.noreply.github.com>

vercel bot had a problem deploying to Preview June 14, 2024 03:54 Failure

fix: add empty hashes where required

e549b4a

vercel bot deployed to Preview June 14, 2024 05:34 View deployment

dai-shi reviewed Jun 19, 2024

View reviewed changes

fix: add href as effect dependency

82b7ae7

Co-authored-by: Daishi Kato <dai-shi@users.noreply.github.com>

vercel bot deployed to Preview July 16, 2024 13:27 View deployment

pmelab added 7 commits July 16, 2024 17:14

fix: rename fixture package

fb2e8a1

refactor: add parameter to parseRoute to ignore the hash

54eec05

Necessary for the initial render, where the hash would cause a hydration mismatch.

Merge branch 'main' into use-router-hash

e4e696e

chore: lockfile update

46a4cae

fix: ignore eslint warning

96a1e4d

chore: upgrade react version of fixture

7822ba5

test: remove redundant parsing of URLSearchparams to verify it breaks

ddb4086

dai-shi mentioned this pull request Jul 23, 2024

breaking(router): string query #799

Merged

dai-shi added a commit that referenced this pull request Jul 24, 2024

breaking(router): string query (#799)

29ca735

#746 revealed that `URLSearchParams` doesn't work. (In the near future, I will also rework on fetchRSC to eliminate it.)

dai-shi mentioned this pull request Jul 25, 2024

v1 Roadmap #24

Open

76 tasks

Merge branch 'main' into use-router-hash

447a150

vercel bot had a problem deploying to Preview July 25, 2024 06:58 Failure

fix: removed searchParams that was left over in merge

efe18a8

vercel bot had a problem deploying to Preview July 25, 2024 07:00 Failure

fix: more merge errors

7928ab7

vercel bot deployed to Preview July 25, 2024 07:08 View deployment

fix: adapt test case

4823571

dai-shi reviewed Jul 25, 2024

View reviewed changes

Update packages/waku/src/router/client.ts

c0d1d24

Co-authored-by: Daishi Kato <dai-shi@users.noreply.github.com>

vercel bot had a problem deploying to Preview July 25, 2024 09:38 Failure

fix: variable naming

a21b8fb

vercel bot deployed to Preview July 25, 2024 09:40 View deployment

docs: add comment on route initialization

2369845

vercel bot deployed to Preview July 25, 2024 11:52 View deployment

docs: typo

bafbc17

vercel bot deployed to Preview July 25, 2024 11:53 View deployment

dai-shi reviewed Jul 25, 2024

View reviewed changes

avoid dual rendering if hash===""

b6638ee

vercel bot deployed to Preview July 25, 2024 14:45 View deployment

Merge branch 'main' into use-router-hash

5c5c64c

vercel bot deployed to Preview July 25, 2024 14:48 View deployment

dai-shi approved these changes Jul 25, 2024

View reviewed changes

dai-shi merged commit d921631 into dai-shi:main Jul 25, 2024
28 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: expose hash from `useRouter` #746

feat: expose hash from `useRouter` #746

pmelab commented Jun 13, 2024 •

edited

Loading

vercel bot commented Jun 13, 2024 •

edited

Loading

codesandbox-ci bot commented Jun 13, 2024 •

edited

Loading

dai-shi left a comment

dai-shi Jun 13, 2024

pmelab Jun 14, 2024

dai-shi Jun 19, 2024

pmelab Jul 17, 2024

pmelab Jul 18, 2024

dai-shi Jul 23, 2024

dai-shi Jul 23, 2024

pmelab commented Jun 14, 2024

dai-shi left a comment

dai-shi Jun 19, 2024

dai-shi Jun 19, 2024

dai-shi commented Jul 24, 2024

dai-shi left a comment •

edited

Loading

dai-shi Jul 25, 2024

pmelab Jul 25, 2024

dai-shi Jul 25, 2024

pmelab Jul 25, 2024

dai-shi left a comment

dai-shi Jul 25, 2024

dai-shi Jul 25, 2024

dai-shi commented Jul 25, 2024

dai-shi left a comment

	const input = getInputString(pathname);
	const body = createElement(
	ServerRouter as FunctionComponent<
	Omit<ComponentProps<typeof ServerRouter>, 'children'>
	>,
	{ route: { path: pathname, searchParams } },
	componentIds.reduceRight(
	(acc: ReactNode, id) => createElement(Slot, { id, fallback: acc }, acc),
	null,
	),
	);
	return { input, body };

	export function ServerRouter({
	children,
	route,
	}: {
	children: ReactNode;
	route: RouteProps;
	}) {
	return createElement(
	Fragment,
	null,
	createElement(
	RouterContext.Provider,
	{
	value: {
	route,
	changeRoute: notAvailableInServer('changeRoute'),
	prefetchRoute: notAvailableInServer('prefetchRoute'),
	},
	},
	children,
	),
	);
	}

feat: expose hash from useRouter #746

feat: expose hash from useRouter #746

Conversation

pmelab commented Jun 13, 2024 • edited Loading

What was changed?

Why was it changed?

Notes

vercel bot commented Jun 13, 2024 • edited Loading

codesandbox-ci bot commented Jun 13, 2024 • edited Loading

dai-shi left a 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

pmelab commented Jun 14, 2024

dai-shi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dai-shi commented Jul 24, 2024

dai-shi left a comment • 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

dai-shi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dai-shi commented Jul 25, 2024

dai-shi left a comment

Choose a reason for hiding this comment

feat: expose hash from `useRouter` #746

feat: expose hash from `useRouter` #746

pmelab commented Jun 13, 2024 •

edited

Loading

vercel bot commented Jun 13, 2024 •

edited

Loading

codesandbox-ci bot commented Jun 13, 2024 •

edited

Loading

dai-shi left a comment •

edited

Loading