Pagination broke in 1.9.0

darylknight commented 2 years ago

Describe the bug

I'm sorry I can't give a huge amount of detail, but I've just gone through the past 5 versions of Sprig to work out where it happened. In 1.8.1 our code for displaying a paginated list of entries (also displayed on a Google Maps plugin map) worked fine. Since updating to 1.9.0 (and 1.9.1, 2, 3). clicking the pagination buttons just scrolls to the top of the page instead of paginating the sprig list.

Wrapper template

{# Map + Pagination #}
{{ sprig('_includes/projectsMap') }}

{# Load Sprig's htmx requirement from a CDN #}
{{ sprig.script }}

{# When sprig filters the results, update the map #}
<script>
    htmx.on('htmx:afterSettle', function(event) { googleMaps.init('projects-map'); });
</script>

_includes/projectsMap

{# Sets a default value if not defined by `s-val:*` on the clicked element #}
{% set page = page ?? 1 %}

{# Search parameters from the URL come from the form fields below the map #}
{% set searchTitle = craft.app.request.getParam('searchTitle')|default('') %}
{% set searchCategory = craft.app.request.getParam('searchCategory')|default('') %}
{% set sort = craft.app.request.getParam('sort')|default('projectSwitchOnDate desc') %}

{# Set up basic query params #}
{% set queryParams = {
    section: 'projects',
    with: ['projectCategories']
} %}

{# Set up an empty array for use with related parameters #}
{% set relatedParams = [] %}

{# ? Run through each search field and add to the query #}

{# * Title #}
{% if searchTitle is not empty %}
  {% set queryParams = queryParams|merge({ search: { query: 'title:' ~ searchTitle } }) %}
{% endif %}

{# * Sort #}
{% if sort is not empty %}
  {# Sort order #}
  {% set queryParams = queryParams|merge({ orderBy: sort }) %}
{% else %}
  {# If no sort order is selected, default to most recent #}
  {% set queryParams = queryParams|merge({ orderBy: 'projectSwitchOnDate desc' }) %}
{% endif %}

{% set mapSearchParams = { components: { country: 'US' }} %}

{# * Type/Category #}
{% if searchCategory is not empty %}
  {# Set the category object from the URL parameter #}
  {% set searchCategoryObject = craft.categories.group('projects').slug(searchCategory).one() %}
  {% set relatedParams = [ { targetElement: searchCategoryObject } ] %}
{% endif %}

{# If a category is selected, merge the relatedParams set above into the query #}
{% if relatedParams|length %}
  {% set queryParams = queryParams|merge({ relatedTo: relatedParams }) %}
{% endif %}

{# ? Don't add .all() here because the list view uses the same query, and can't have .all() on it #}
{% set locations = craft.entries(queryParams).projectAddress(mapSearchParams) %}

<form sprig>
  <label for="searchTitle" class="visually-hidden">Search projects</label>
  <input sprig s-trigger="keyup changed delay:500ms" s-replace="#sprig-map-results" type="text" class="form-control" name="searchTitle" id="searchTitle" placeholder="Search projects" value="{{ searchTitle }}" />

  <label class="d-inline-block font-heading text-blue-dark me-1">Filters</label>
  <select sprig s-replace="#sprig-map-results" name="searchCategory" id="searchCategory" class="form-select">
    <option value=""> Select project type </option>

    {% for cat in craft.categories.group('projects').all() %}
      <option value="{{ cat.slug }}" {{ cat.slug == searchCategory ? 'selected' }}>
        {{ cat.title }}
      </option>
    {% endfor %}
  </select>

  <label class="d-inline-block font-heading text-blue-dark me-1">Sort</label>
  <select sprig s-replace="#sprig-map-results" name="sort" id="sort" class="form-select">
    <option value="title asc" {{ sort == 'title asc' ? 'selected' }}>
      Project: A-Z
    </option>
    <option value="title desc" {{ sort == 'title desc' ? 'selected' }}>
      Project: Z-A
    </option>
  </select>
</form>

{# Create the options for map appearance #}
{% set mapRenderOptions = {
  id: 'projects-map',
  height: 650,
  cluster: {
    renderer: 'CustomRenderer',
    imagePath: '/assets/images/map-clusters/m'
  },
  mapOptions: {
    streetViewControl: false,
    mapTypeControl: false
  },
  infoWindowTemplate: '_includes/infoWindow'
} %}

{# Create the map object with the locations and options #}
{# ? Add .all() to this locations array so the map works #}
{% set map = googleMaps.map(locations.all(), mapRenderOptions) %}

<div id="sprig-map-results">
  {# Render the map #}
  {{ map.tag() }}

  {# ? https://putyourlightson.com/plugins/sprig#sprig.paginateelementquery-page #}
  {# ? https://putyourlightson.com/sprig/cookbook#pagination #}

  {# Set pageInfo using Sprig so pagination is reactive #}
  {% set entryQuery = craft.entries(queryParams).limit(10) %}

  {% set pageInfo = sprig.paginate(entryQuery, page) %}
  {% set entries = pageInfo.pageResults %}

  <table class="table mb-6">
    {% for project in entries %}
      {{ project.title }}
    {% endfor %}
  </table>

  {% if page > 1 %}
    {# If this isn't the first page, show the back button #}
    <a href="#" sprig s-val:page="{{ page - 1 }}" s-push-url="?page={{ page - 1 }}">Prev</a>
  {% endif %}
  {% for i in pageInfo.getDynamicRange(3) %}
    {% if i == page %}
      <span class="current">{{ i }}</span>
    {% else %}
      {# Refreshes the component and pushes the new value into the URL #}
      <a sprig s-val:page="{{ i }}" href="#" s-push-url="?page={{ i }}">{{ i }}</a>
    {% endif %}
  {% endfor %}
  {% if page < pageInfo.totalPages %}
    {# If this isn't the last page, show the next button #}
    <a href="#" sprig s-val:page="{{ page + 1 }}" s-push-url="?page={{ page + 1 }}">Next</a>
  {% endif %}
</div>

Expected behaviour

Pagination should switch pages without refreshing the page, like Sprig pagination normally does. Works fine on 1.8.1. In 1.9.0+, clicking a pagination number scrolls the page up (because of the #), but doesn't paginate.

Versions

Plugin version: Works in 1.8.1. Breaks in 1.9.0+
Craft version: 3.7.16

bencroker commented 2 years ago

What happens if you remove all of the href="#" attributes?

darylknight commented 2 years ago

Then it's not recognised as something you can click on, and does nothing. It also doesn't work in 1.9.0 if I convert those a tags to buttons

bencroker commented 2 years ago

The recipe at https://putyourlightson.com/sprig/cookbook#pagination is on the latest version of Sprig and working fine. Can you test on your end using the recipe and then compare the differences? Also be sure to look out for errors in the browser console and network tab.

darylknight commented 2 years ago

Looks like it basically all works ok (even with a href="#", without converting them to buttons) if I take the map out, by commenting out {{ map.tag() }}.

Looping in @lindseydiloreto - could either of you think of something that changed in 1.9.0 that would break the pagination if there's a Google Map on the page?

bencroker commented 2 years ago

Maybe something changed in htmx, you could try loading version 1.5.0 to see if that makes a difference. https://unpkg.com/htmx.org@1.5.0/dist/htmx.min.js

lindseydiloreto commented 2 years ago

I'm about 80% certain that it's not a problem with Google Maps, though I'm not sure I know enough about Sprig to be helpful.

You can try (1) commenting out the entire map section, and (2) ditching the htmx:afterSettle event.

Let us know if you're still having problems after removing those two things.

darylknight commented 2 years ago

Yep, just tested it again, all on Sprig 1.10.4:

If I remove {{ map.tag() }} from the code, pagination works again. That's why I looped in Lindsey. Although why that would be the case between 1.8.1 and 1.9.0+ I don't know.
If only htmx.on('htmx:afterSettle', function(event) { googleMaps.init('projects-map'); }); is removed, but the map is left in, pagination is broken
changing htmx to 1.5.0 in SprigVariable.php, doesn't fix it

bencroker commented 2 years ago

Sounds like you're going have to examine the JS code run by the maps tag to see why it conflicts with htmx. Are no errors reported in the console?

darylknight commented 2 years ago

No, there are no errors in the javascript console or in Craft

darylknight commented 2 years ago

For what it's worth - this is still broken in the latest version of Sprig. I've been doing more testing for a few hours this morning, but am currently having to still leave Sprig at 1.8.1 to get it to work.

What is weird, is that it seems to all work fine if I have "roughly less than 150" entries - but at some point at 100+, changing the filters just does nothing. No errors normally selecting something in the dropdown filters the map, but at an unspecified amount of entries, that interactivity stops working and doesn't seem to make any kind of HTML call. Could there be something in Sprig (or again, in combination with Google Maps) that makes it stop working if the query is too long, or too large?

bencroker commented 2 years ago

I believe there is a known issue with the Google Maps plugin, perhaps https://github.com/doublesecretagency/craft-googlemaps/issues/52?

darylknight commented 2 years ago

Thanks Ben - that's good to know. However, fixing Google Maps at 4.1.4 and updating Sprig to latest still has the same results (but again, it works fine if the map isn't rendered, so I doubt it's just Sprig). Will leave it alone for another few months and see if anything changes in Craft 4.

bencroker commented 2 years ago

Maybe there's something else at play here, but I remember that @lindseydiloreto was working on a fix for another similar issue.

lindseydiloreto commented 2 years ago

@bencroker is correct, this is probably more of a Google Maps issue than a Sprig issue (though it's still not 100% certain).

We ran into a problem where someone had an insane amount of markers (thousands), each with an attached info window. Prior to v4.1.5, all of the info window data was being compacted into the data-dna of the map container. Unfortunately, this caused a conflict with Sprig... It made the div tag absolutely enormous, and Sprig stopped processing it (similar to what you described).

So in v4.1.5, we refactored how info windows are handled. The pre-rendered HTML for info windows is no longer stored within the data-dna attribute, those snippets are now stored outside of the div in a simple JavaScript variable.

Unfortunately, this introduced a couple of minor regression bugs. With the exception of one, all the regression issues have been fixed. But this one might be a little more complicated to sort out.

Due to timing and other factors, we were pushed to update the Google Maps plugin to be compatible with Craft 4, without having a chance to properly fix this issue first. When a fix for this is introduced, it will likely only be available on Craft 4.

@darylknight I will ping you via Discord DM to figure out what the best next step is. At some point we will be releasing a proper fix for this issue, but that may still be several weeks away.

darylknight commented 2 years ago

Thanks both! Chatted to Lindsey on Discord and decided to leave it on Sprig 1.8.1 and Google Maps 4.1.4 for now, until GM is updated for Craft 4. After that I'll revisit this and see if it's still a problem on the latest version of both plugins or if the updates fix it.

lindseydiloreto commented 2 years ago

@darylknight This is fixed in v4.2.3 of the Google Maps plugin. As noted earlier, it requires Craft 4.

Go ahead and update when you get a chance, let me know if i missed anything. 👍

bencroker commented 2 years ago

Thanks Lindsey!

darylknight commented 2 years ago

I've updated my code example in the original post (and stripped it down as much as possible).

Ok, I'm all updated to Craft 4 and the latest versions of both plugins, still having the same problem. I think I've tracked it down to an inconsistency between the required query for the two kinds of data I'm trying to display.

For context, here's the URL this relates to: https://www.solvenergy.com/projects (this live page is still on Craft 3 using the combination of plugins that still works). Scroll down to the map section with the filter bar.

The Google Map above needs to show all 500+ projects. The Google Maps plugin requires calling .all() on the query (docs)
There's a paginated list underneath, which is set to 10. This requires NOT calling .all() on the entry query (docs)

So after updating everything, here's what I've tried:

Current code version - includes .limit(10) on the pagination query

Clicking the pagination buttons scrolls back to the top of the page
Clicking the filters does nothing. No network event is sent by Sprig

If I remove the .limit(10) from the pagination query:

List below shows 100 projects at a time
Pagination doesn't work
Filters don't work

If I remove .limit(10) from the pagination query, and add limit:10, to queryParams at the top of the page:

the map only shows 10 entries
pagination works
filters work on the map (but only shows 10)

If I update these two lines and move the .all() call, and keep the set entryQuery line as it is, then we're back to pagination not working (when it worked in Sprig 1.8.1)

{% set locations = craft.entries(queryParams).projectAddress(mapSearchParams).all() %}
{% set map = googleMaps.map(locations, mapRenderOptions) %}

So... I've spent probably 20-30 hours trying to fix this since the Sprig update to 1.9.0 now - I have no idea what changed, but something in that update broke the pagination here. I can't ask you two to trawl through the code to work out what's going on either.

I guess the simplest question is whether this design / layout is practical with these two plugins anymore? Should I just separate these two things so that the map always shows all entries, and the list view underneath is filterable?

bencroker commented 2 years ago

My guess is that the response is so large that Sprig is choking during the parsing process. I'm seeing upwards of 2 MB to load all map markers, which seems excessive (and slow). This might be a Sprig issue, but I'd start by figuring out whether those responses can be optimised, which I'm guessing is on the Google Maps plugin.

darylknight commented 2 years ago

That's possible. I've just realised we're using pretty much exactly the same code on the homepage of danco-group.com, and that has a map that shows all entries, with a paginated list underneath. That's on Google Maps 4.1.9 / Sprig 1.13.0 and it works fine with the same pagination. Only 35 entries though.

lindseydiloreto commented 2 years ago

@darylknight Not sure if this exactly helps you, but I'm currently working on a project which has a map (showing all results) alongside of a paginated set of results (limit 10). To pull that off, we cloned the query to have paginated and non-paginated queries.

{# Split into parallel queries #}
{% set fullQuery = clone(query) %}
{% set paginatedQuery = query.limit(10) %}

darylknight commented 2 years ago

Thanks Lindsey. I've just tried that and have the same result - pagination scrolls back up the page, and the filters don't do anything.

putyourlightson / craft-sprig