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

Improve missing data fallback #258

Merged
merged 10 commits into from
Jan 3, 2025
Merged

Conversation

alecgeatches
Copy link
Contributor

@alecgeatches alecgeatches commented Dec 20, 2024

This PR addresses data fallback issues in three scenarios:

1. Single-item request errors

This scenario is set up using an Art Institute of Chicago block:

art-post

To mock a failed API request, we'll change the endpoint configuration to something invalid:

  'endpoint' => function ( array $input_variables ) use ( $aic_data_source ): string {
-      return sprintf( '%s/%s', $aic_data_source->get_endpoint(), $input_variables['id'] ?? '' );
+      return sprintf( '%s/%s/invalid-endpoint', $aic_data_source->get_endpoint(), $input_variables['id'] ?? '' );
  },

On trunk, the remote data block fails to load with this change:

art-failed-load

In this branch (fix/missing-data-fallback), we can fix the issue by using stored result data in the block's context. We return the result data from the get_value() fallback, which fixes the issue:

art-fallback-load


2. Single-item removed block

Using the same post as above, we'll instead remove the code that registers the block:

  function load_only_if_parent_plugin_is_active() {
      /* ... */
  
      require_once __DIR__ . '/airtable/elden-ring-map/register.php';
      require_once __DIR__ . '/github/remote-data-blocks/register.php';
-     require_once __DIR__ . '/rest-api/art-institute/register.php';
      require_once __DIR__ . '/rest-api/zip-code/register.php';
      require_once __DIR__ . '/shopify/register.php';
      require_once __DIR__ . '/google-sheets/westeros-houses/register.php';
  }

On trunk this fails to load as before. The fix used in scenario 1 doesn't work here, because the block is unregistered, which means no usesContext is registered, and we no longer have access to parent context to fill data values from saved remote-data-blocks/remoteData.

The new code returns null instead of an empty string, which allows WordPress to ignore the get_value() call and used the stored attribute value. This fixes rendering as above.

3. Multiple-item request errors

For this scenario, we'll add a Westeros Houses List to the editor:

got-editor

Next, we'll change the spreadsheet ID to something invalid:

  'spreadsheet' => [
-     'id' => '1EHdQg53Doz0B-ImrGz_hTleYeSvkVIk_NSJCOM1FQk0',
+     'id' => '123',
  ],

This causes the front-end rendering to break in trunk:

got-broken

This is fixed in this PR using the same logic as the first scenario, using the embedded result data from context with an associated item index. The same post in fix/missing-data-fallback:

got-fixed


Notes

There are currently two data fallback issues not covered by this PR:

  1. Multiple-item removed block, e.g. removing the Westeros Houses List Block registration. We can't use the same strategy as scenario 2 (use saved attributes) because we only have one saved block, the template:

    image

    The rest of the items in the list only exist in the saved results header, and it's up to code to loop through these to generate the rest of the markup. We also can't use the fix from scenario 3 (use the embedded results) because the block is unregistered and no longer has access to parent context.

    I think this may be fixable by detecting an unregistered remote-data/ block and injecting the ability to use remote-data-blocks/remoteData context. However I'd like to address that in a separate PR, as it'll probably be more complex.

  2. I noticed when testing scenario 2 with a Shopify block that content still doesn't load. It appears to be because we don't actually store any attributes on Shopify item markup and only provide it via context. For this item:

    shirt-editor

    we have this markup in editor:

    <!-- wp:remote-data-blocks/shopify-example {"remoteData":{"blockName":"remote-data-blocks/shopify-example","isCollection":false,"metadata":{"last_updated":{"name":"Last updated","type":"string","value":"2024-12-20 18:05:38"},"total_count":{"name":"Total count","type":"integer","value":1}},"queryInput":{"id":"gid://shopify/Product/8495263088864"},"resultId":"84c69d4a-8c5b-4deb-b7ef-832f836b937d","results":[{"description":"Your Toddler deserves t-shirts with the same legendary wit they themselves posses. This super soft 100% cotton, relaxed-fit t-shirt is bound to become your Toddler’s favorite shirt to cover in stains.","details_button_url":"/path-to-page/gid://shopify/Product/8495263088864","image_alt_text":"","image_url":"https://cdn.shopify.com/s/files/1/0680/3456/0224/files/mockup-c4f39ad8.jpg?v=1716336804","price":"$20.00","title":"'Toddler Leaders Call For Increased Duck Visibility' Toddler T-Shirt","variant_id":"gid://shopify/ProductVariant/45613761069280"}]}} -->
    <div class="wp-block-remote-data-blocks-shopify-example rdb-container">
      <!-- wp:group {"metadata":{"categories":["Remote Data"],"patternName":"remote-data-blocks/shopify-product-teaser","name":"Shopify Product Teaser"},"layout":{"type":"constrained"}} -->
      <div class="wp-block-group"><!-- wp:columns -->
        <div class="wp-block-columns"><!-- wp:column -->
          <div class="wp-block-column">
            <!-- wp:image {"metadata":{"bindings":{"alt":{"source":"remote-data/binding","args":{"field":"image_alt_text","block":"remote-data-blocks/shopify-example"}},"url":{"source":"remote-data/binding","args":{"field":"image_url","block":"remote-data-blocks/shopify-example"}}},"name":"Image URL"}} -->
            <figure class="wp-block-image"><img src="" alt="" /></figure>
            <!-- /wp:image -->
    
            <!-- wp:paragraph {"metadata":{"bindings":{"content":{"source":"remote-data/binding","args":{"field":"price","block":"remote-data-blocks/shopify-example"}}},"name":"Item price"}} -->
            <p></p>
            <!-- /wp:paragraph -->
          </div>
          <!-- /wp:column -->
    
          <!-- wp:column -->
          <div class="wp-block-column">
            <!-- wp:heading {"metadata":{"bindings":{"content":{"source":"remote-data/binding","args":{"field":"title","block":"remote-data-blocks/shopify-example"}}},"name":"Title"}} -->
            <h2 class="wp-block-heading"></h2>
            <!-- /wp:heading -->
    
            <!-- wp:paragraph {"metadata":{"bindings":{"content":{"source":"remote-data/binding","args":{"field":"description","block":"remote-data-blocks/shopify-example"}}},"name":"Product description"}} -->
            <p></p>
            <!-- /wp:paragraph -->
          </div>
          <!-- /wp:column -->
        </div>
        <!-- /wp:columns -->
      </div>
      <!-- /wp:group -->
    </div>
    <!-- /wp:remote-data-blocks/shopify-example -->

    Note that the <img>, <p>, and <h2> tags are all empty and rely on context to fill values. I'm not sure why the Shopify block specifically is acting this way and not storing values in markup, but the fix may be similar to the prior issue where we need to pull context in unregistered blocks. I'll also look into this in another PR.

    Small update: It's also possible to recreate this behavior with other blocks by manually creating a pattern instead of selecting it. It seems like we're only injecting fallback attributes when a pattern is selected from automatic generation.

@alecgeatches alecgeatches self-assigned this Dec 26, 2024
@alecgeatches
Copy link
Contributor Author

I'm working on tests, but these may be a bit disruptive and large to add to this PR. I'm going to work on those in a separate PR. This is ready for review!

@alecgeatches alecgeatches marked this pull request as ready for review January 2, 2025 16:37
@maxschmeling maxschmeling merged commit e3dd2d5 into trunk Jan 3, 2025
11 checks passed
@maxschmeling maxschmeling deleted the fix/missing-data-fallback branch January 3, 2025 22:01
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

Successfully merging this pull request may close these issues.

2 participants