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

Incorrect query arguments handling in fetchPreRenderData #1455

Open
rinatkhaziev opened this issue Oct 29, 2020 · 5 comments
Open

Incorrect query arguments handling in fetchPreRenderData #1455

rinatkhaziev opened this issue Oct 29, 2020 · 5 comments

Comments

@rinatkhaziev
Copy link

rinatkhaziev commented Oct 29, 2020

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 rinatkhaziev changed the title Incorrect query arguments handling fetchPreRenderData Incorrect query arguments handling in fetchPreRenderData Oct 29, 2020
@rinatkhaziev
Copy link
Author

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

@prateekbh
Copy link
Member

@rinatkhaziev are you saying this is an invalid issue?

@rinatkhaziev
Copy link
Author

rinatkhaziev commented Nov 1, 2020

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

@mjmaurer
Copy link

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

@rinatkhaziev
Copy link
Author

@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('&');

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

No branches or pull requests

3 participants