-
Notifications
You must be signed in to change notification settings - Fork 8
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
Turning off prefetch for pages that not in the routes list #20
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @SilentImp, yes good idea, thanks for your PR. I'd just make it as an option.
src/index.js
Outdated
@@ -41,6 +41,7 @@ export default class UrlPrettifier<T: string> { | |||
currentRoute.page === pageName)[0]; | |||
return { | |||
href: `/${pageName}${this.paramsToQueryString(params)}`, | |||
...(!route && {prefetch: false}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not confortable with a lib changing default params for other libs, so we could pass it as an option, what do you think?
Something like:
new UrlPrettifier(routes, {preventPrefetchIfNoRoute: true})
(preventPrefetchIfNoRoute
being false
by default)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sound nice!
I will update an PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't quite like that you need to pass a property with such a long name, presumably each time you are using component … but I can't come with better naming, so I guess it's a good solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BDav24 updated PR :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't quite like that you need to pass a property with such a long name, presumably each time you are using component …
Yes my first idea was to pass the param to the lib entrypoint new UrlPrettifier(routes, {preventPrefetchIfNoRoute: true})
, so you don't have to pass it each time ;)
98efda5
to
d2d707c
Compare
Hi.
Maybe it's a nice idea to turn off
prefetch
(which is default totrue
) if we don't have a page in the routes list?With all best regards. Anton.