Skip to content
This repository has been archived by the owner on Jan 11, 2023. It is now read-only.

Use sapper:prefetch and sapper:external instead of rel attribute #1427

Closed
wants to merge 3 commits into from
Closed

Use sapper:prefetch and sapper:external instead of rel attribute #1427

wants to merge 3 commits into from

Conversation

arekbartnik
Copy link
Contributor

@arekbartnik arekbartnik commented Aug 22, 2020

Addtionally to reasons stated in #1401 for rel=prefetch I think the sapper:[action] is a better pattern for all actions on links in a Sapper app. Where actions are: prefetch, external or noscroll.

It's a breaking change.

I've also updated svelte-language tools sveltejs/language-tools#475

Closes #1401

<!-- TODO add a function to prefetch programmatically -->

### rel=external
Copy link
Member

Choose a reason for hiding this comment

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

I'm not as sure that we should change this one. We're using rel=external as intended

Copy link
Contributor Author

@arekbartnik arekbartnik Aug 22, 2020

Choose a reason for hiding this comment

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

I've recently needed to navigate to a site in same-origin which wasn't a part of a Sapper app. Without rel=external it's not possible.
My argumentation is that application actions should not be bound to a existing attributes (in long term this always creates problems - like with rel=prefetch).

Copy link
Member

Choose a reason for hiding this comment

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

I've recently needed to navigate to a site in same-origin which wasn't a part of a Sapper app. Without rel=external it's not possible.

I would think we should just make the router fall through and treat a link as a normal link in the case that it can't find a corresponding route

Copy link
Member

Choose a reason for hiding this comment

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

I'm confused about what specific case has an issue with treating rel=external in this way. I believe Sapper's behavior for the click interception is based on that of TJ's Page.js. In any case, IIRC Sapper already does bail on the interception if the link isn't to something that matches any route.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Conduitry my example:

  • the main page is an Sapper app with link to articles at example.com
  • articles aren't part of the Sapper app but are availble at example.com/category/article-title

Maybe both options should be available?

Copy link
Member

Choose a reason for hiding this comment

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

Tries to load it and then what happens? You get a 404? What do you mean that use use [...slug].svelte?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In routes I have [...slug].svelte to map all pathnames. So Sapper thinks there is a route for an article and after click loads template from [...slug].svelte with an error.
Then after page refresh VIP directs browser to a different app because articles are served by this app and not by Sapper.

I admit - it's a highly specific case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe I should reduce this PR to a problem from #1401 and then open new issue about potential changes to rel=external?

Copy link
Member

Choose a reason for hiding this comment

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

I still don't know what that has to do with making rel=external not have the normal page reload nav behavior. The idea is that target or download or rel=external will make it a non SPA link even if it matches some route.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Conduitry True, I fixated on my case and shouldn't remove functionality for rel=external.

I've made PR only for the prefetch #1566 and will close this one.

@arekbartnik arekbartnik mentioned this pull request Sep 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
breaking Breaking Changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename rel=prefetch to sapper:prefetch
3 participants