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

New global option: resolvePath #512

Merged
merged 17 commits into from Nov 7, 2022
Merged

New global option: resolvePath #512

merged 17 commits into from Nov 7, 2022

Conversation

hirasso
Copy link
Member

@hirasso hirasso commented Sep 12, 2022

Replaces #508

Adds a new global option to swup, that allows to run a callback for how swup should resolve a path.

What is the problem?

Right now, to be able to leave and re-enter control over the browser URL/state by swup, three steps are necessary:

  • Use skipPopStateHandling with custom logic
  • Manually ignore links with one of the given options (linkSelector, stopPropagation() during the capture phase,...)
  • Manually adjust the cache URLs so that back- and forward navigation doesn't need additional server requests

While this kind of behavior is already possible to achieve with the currently available swup API, it adds a lot of boilerplate to each new site that wants to have this. Also, the current cache implementation is less than ideal for that kind of scenario right now. It will either fill itself up with unnecessary duplicates of the same page or have cache misses for pages that were previously already loaded from the server.

Proposed solution

All three of the above topics basically boil down to always the same question: Should various paths actually be resolved to just one by swup?

  • on popState: Is the resolved version of the new browser URL the same as the previous one? > ignore
  • on clickLink: Is the resolved href of the link the same as the current browser URL? > ignore
  • Accordingly to swup ignoring the above events if paths resolve to the same one, the cache can also just keep one copy for all URLs that resolve to the same one.

Demo & Test Site

I have set-up a test site here: https://test.rassohilber.com/swup-resolve-path/test/

On that site, the whole filter logic is done by a custom component that doesn't want swup to interfere. It does everything from updating the URL to rendering the state. Navigation away from the list to one of the character detail pages or to the "about"-page and back again is being handled by swup again. To make it more obvious that there is no page load between filters, I added a staggered animation to the items that only runs if the page is being fully rendered (initially and by swup).

Code Example

const swup = new Swup({
  resolvePath: path => {
    // resolve every URL that contains '/?=filter=xyz' to '/'
    return path.replace(/\/\?filter=.+/, '/');
  }
})

Checks

  • The code was linted before pushing
  • All tests are passing
  • New or updated tests are included
  • The documentation was updated as required
  • Make sure that restoring scroll positions in ScrollPlugin also uses the resolved path

@cypress
Copy link

cypress bot commented Sep 12, 2022



Test summary

27 0 0 0


Run details

Project swup
Status Passed
Commit 1e6ad7c
Started Nov 7, 2022 1:23 PM
Ended Nov 7, 2022 1:24 PM
Duration 00:31 💡
OS Linux Debian - 10.5
Browser Electron 102

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@daun
Copy link
Member

daun commented Sep 12, 2022

Nice! Will check it out this week.

@daun
Copy link
Member

daun commented Sep 12, 2022

One thought beforehand: returning null from the resolver function will result in a warning. I think returning null or undefined could have a meaning here as well, i.e. "leave the url as-is". Probably no need to throw a warning, then?

`null`, `false`, `undefined` or `''` will all result in a warning now to help with the implementation of this option.
@hirasso
Copy link
Member Author

hirasso commented Sep 12, 2022

I would actually prefer to be quite strict here, a warning will hopefully help people with the implementation. The function should always return a non-empty string. I actually also thought about validating if the resolved path is actually a path and not a full URL, as well.

@hirasso
Copy link
Member Author

hirasso commented Sep 12, 2022

Could you also have a look at the tests? They are passing locally, I don't understand what's going on in the CI 🙏

@hirasso hirasso marked this pull request as ready for review September 12, 2022 19:22
@hirasso hirasso self-assigned this Sep 12, 2022
@daun
Copy link
Member

daun commented Sep 12, 2022

Looks like we reached the monthly limit for the free CircleCI plan, it won't show the test run :(

@hirasso
Copy link
Member Author

hirasso commented Sep 13, 2022

I think it might be the complex transitions test, the duration was a bit outside the expected range one time I let it run locally.

@daun
Copy link
Member

daun commented Sep 13, 2022

You could try increasing the expected range — it's a constant somewhere at the top of main.spec.js, I think 0.1 at the moment. Setting it to 0.15 or so might already help?

@hirasso
Copy link
Member Author

hirasso commented Sep 13, 2022

Great, that worked! 🎺

@hirasso hirasso requested a review from gmrchk September 13, 2022 18:31
@hirasso hirasso mentioned this pull request Sep 30, 2022
4 tasks
@hirasso
Copy link
Member Author

hirasso commented Sep 30, 2022

@daun do you have any reservations against merging this?

@daun
Copy link
Member

daun commented Oct 3, 2022

@hirasso Only that I've been super busy and haven't had the time to thoroughly test this. I hope I can give it a spin later this week or on the weekend.

@daun
Copy link
Member

daun commented Nov 4, 2022

Sorry for the delay in checking this out. Looks good! It's something we should definitely have in the codebase.

One question about the scope: does the linked example always return the same HTML, regardless of the filter param? I'm thinking about the implications of resolving the cache urls as well here.

@hirasso
Copy link
Member Author

hirasso commented Nov 4, 2022

One question about the scope: does the linked example always return the same HTML, regardless of the filter param? I'm thinking about the implications of resolving the cache urls as well here.

Yes, each ?filter=...-page in the example returns the same dataset with all items, regardless of the filter in the URL. The pages only differ in two things that are good SEO practice – the initial document title reflects the selected filter and the items that match the filter are being rendered initially (and then thrown away as soon as my component takes over).

The cache must use resolvePath, as well, if you think about the user journey: They have changed the filter a few times (swup ignores these), then navigated to the "about" page (wich is handled by swup again). If they now would go back in the history, they would expect the previous pages to be in the cache. Without the cache also using resolvePath, there would be no cache records and swup would needlessly have to re-fetch the page from the server.

If you need further clarification, I would be happy to walk you through it in a call – otherwise I would proceed and merge this. Documentation can be a separate PR.

@hirasso
Copy link
Member Author

hirasso commented Nov 5, 2022

Actually, thinking about it: The concept is actually quite similar to canonical URLs:

A canonical URL is the URL of the page that Google thinks is most representative from a set of duplicate pages on your site.

So maybe renaming the option to getCanonicalURL would help with understanding it better, what do you think?

@daun
Copy link
Member

daun commented Nov 5, 2022

Let's jump on a quick call whenever convenient! I agree with the reasoning but would just like to get your input on how I have been solving those filter ui situations. Ideally this PR would cover both ways of tackling it.

I'd stick with the current naming. Calling it canonical would somewhat imply being tied to a hypothetically present meta tag whereas resolve is more neutral.

@hirasso
Copy link
Member Author

hirasso commented Nov 5, 2022

I see... Do you also prefer resolvePath over mapPath?

@daun
Copy link
Member

daun commented Nov 5, 2022

I think I prefer resolve over map as well. To me, mapping implies a 1:1 relationship whereas resolving implies combining as well as rewriting. Neither sounds false, but resolve is the one that'd make me instantly understand the concept when encountering the option in the docs.

How strong is your aversion to using resolve here?

@hirasso
Copy link
Member Author

hirasso commented Nov 5, 2022

Not strong at all. Just wanted to make sure we are naming it correctly. Let's stay with resolve, then! 👏

@hirasso hirasso mentioned this pull request Nov 7, 2022
2 tasks
@hirasso hirasso merged commit b171f0e into next Nov 7, 2022
@daun daun deleted the resolve-path branch January 5, 2023 21:01
@daun daun added this to the v3 milestone Feb 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Released
Development

Successfully merging this pull request may close these issues.

None yet

2 participants