preact-cli icon indicating copy to clipboard operation
preact-cli copied to clipboard

Incorrect query arguments handling in fetchPreRenderData

Open rinatkhaziev opened this issue 3 years ago • 5 comments

Do you want to request a feature or report a bug?

A bug.

What is the current behaviour?

When visiting an URL that has GET query arguments passed (location.search) the resulting preact_prerender_data.json URL will be constructed using the full URL. E.g.

https://example.com/my-route/?some-query-argument=1 will result in the following:

https://example.com/my-route/?some-query-argument=1/preact_prerender_data.json

I'm not sure whether that's a bug or by design, but in either case, it doesn't seem like correct behavior.

What is the expected behaviour?

What probably should happen is normalizeUrl() should strip out the query arguments and they should be appended at the end.

So the resulting URL should be https://example.com/my-route/preact_prerender_data.json?some-query-argument=1

This would allow for more reliable behavior and better flexibility.

  1. For example, Facebook likes to append ?fbclid argument at the end of a shared url - current Implementation will be broken.
  2. Passing the arguments at the end of query args would allow to handle those server-side (E.g.. if preact_prerender_data.json is not a statically generated file but instead just an endpoint.

I'm happy to submit a PR just as long as y'all are interested.

[EDIT]

app.js

import { h } from 'preact';
import { Router } from 'preact-router';
import { Provider } from '@preact/prerender-data-provider';
import Header from './header';

// Code-splitting is automated for `routes` directory
import Home from '../routes/home';
import Profile from '../routes/profile';

const App = (props) => (
	<Provider value={props}>
	<div id="app">
		<Header />
		<Router>
			<Home path="/" />
			<Profile path="/profile/" user="me" />
			<Profile path="/profile/:user" />
		</Router>
	</div>
	</Provider>
)

export default App;

home.js

import { h } from 'preact';
import style from './style.css';
import { usePrerenderData } from '@preact/prerender-data-provider';

const Home = (props) => {
	console.log(props.url);
	const [data, loading, error] = usePrerenderData(props)
	return (
		<div class={style.home}>
		<h1>Home</h1>
		<p>This is the Home component.</p>
	</div>
)};

export default Home;
Screen Shot 2020-11-01 at 1 45 33 PM

rinatkhaziev avatar Oct 29 '20 19:10 rinatkhaziev

The issue was with me passing down the url prop from preact-router which contains the query string, hence breaking the logic.

rinatkhaziev avatar Nov 01 '20 01:11 rinatkhaziev

@rinatkhaziev are you saying this is an invalid issue?

prateekbh avatar Nov 01 '20 07:11 prateekbh

@prateekbh no, it's a valid issue, the last message was mostly a note to myself, apologies for the confusion.

Let me illustrate, I just created a fresh app with the default template:

app.js

import { h } from 'preact';
import { Router } from 'preact-router';
import { Provider } from '@preact/prerender-data-provider';
import Header from './header';

// Code-splitting is automated for `routes` directory
import Home from '../routes/home';
import Profile from '../routes/profile';

const App = (props) => (
	<Provider value={props}>
	<div id="app">
		<Header />
		<Router>
			<Home path="/" />
			<Profile path="/profile/" user="me" />
			<Profile path="/profile/:user" />
		</Router>
	</div>
	</Provider>
)

export default App;

home.js

import { h } from 'preact';
import style from './style.css';
import { usePrerenderData } from '@preact/prerender-data-provider';

const Home = (props) => {
	console.log(props.url);
	const [data, loading, error] = usePrerenderData(props)
	return (
		<div class={style.home}>
		<h1>Home</h1>
		<p>This is the Home component.</p>
	</div>
)};

export default Home;
Screen Shot 2020-11-01 at 1 45 33 PM

I have side-stepped the issue by adding a helper function that I use like so:

let [ data, loading, error ] = usePrerenderData( { ...props, url: queryless`${props.url}` })

It works but doesn't feel great :)

and the relevant code in queryless is

str.split('?',1)[0]

PS I've edited the original issue message to include the details above.

rinatkhaziev avatar Nov 01 '20 19:11 rinatkhaziev

@rinatkhaziev did you end up making a PR? I might take this on if not.

mjmaurer avatar Jun 30 '21 05:06 mjmaurer

@mjmaurer to my greatest shame, I have not!

An example of what I have in mind is this, but there are tons of other ways one can do it.

let url = document.location.href.split('?');
let [ base, ...query ] = url;
let formattedUrl =  base + '/preact_prerender_data.json?' + query.join('&');

rinatkhaziev avatar Jul 02 '21 03:07 rinatkhaziev