Skip to content

invalidate callback should take a { force: boolean } optional param #166

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

Closed
bennypowers opened this issue Jun 1, 2021 · 0 comments · Fixed by #167
Closed

invalidate callback should take a { force: boolean } optional param #166

bennypowers opened this issue Jun 1, 2021 · 0 comments · Fixed by #167

Comments

@bennypowers
Copy link
Contributor

bennypowers commented Jun 1, 2021

Hi! 👋

Firstly, thanks for your work on this project! 🙂

Today I used patch-package to patch [email protected] for the project I'm working on.

A hybrids element's observed property may be an object with it's own internal state. Such is the case in my project, the object is a lit ReactiveController. Hybrids version 5.2.1 allowed me to notify the host element of internal changes via calls to invalidate but as of 5.3.4, invalidate calls no longer trigger re-render:

https://github.com/apollo-elements/apollo-elements/runs/2717883033?check_suite_focus=true

import { expect, fixture, nextFrame } from '@open-wc/testing';
import { define, html, Hybrids } from 'hybrids';
import { mutation } from './mutation';

let counter = 0;

describe('[hybrids] mutation factory', function() {
  describe('with global client', function() {
    describe('with NullableParamMutation controller at "mutation" key', function() {
      interface H extends HTMLElement, TestableElement {
        mutation: ApolloMutationController<typeof S.NullableParamMutation>
      }

      let element: H;

      beforeEach(async function() {
        const tag = getTagName();

        define(tag, {
          mutation: mutation(S.NullableParamMutation),
          render: (host: H) => html`
            <output id="called">${stringify(host.mutation.called)}</output>
            <output id="data">${stringify(host.mutation.data)}</output>
            <output id="error">${stringify(host.mutation.error)}</output>
            <output id="errors">${stringify(host.mutation.errors)}</output>
            <output id="loading">${stringify(host.mutation.loading)}</output>
          `,
        } as Hybrids<H>);

        element = await fixture<H>(`<${tag}></${tag})`);
      });

      describe('calling mutate', function() {
        let p: Promise<any>;
        beforeEach(function() {
          // THIS WILL UPDATE `element.mutation`'s INTERNAL STATE
          p = element.mutation.mutate({ variables: { delay: 200 } });
        });

        beforeEach(nextFrame);

        it('renders loading state', function() {
          // FAILS: `element.mutation` identity did not change, so `invalidate()` did not rerender
          expect(element).shadowDom.to.equal(`
            <output id="called">true</output>
            <output id="data">null</output>
            <output id="error">null</output>
            <output id="errors">[]</output>
            <output id="loading">true</output>
          `);
        });
      });
    });
  });
});

Here is the diff that solved my problem:

diff --git a/node_modules/hybrids/.DS_Store b/node_modules/hybrids/.DS_Store
new file mode 100644
index 0000000..5172429
Binary files /dev/null and b/node_modules/hybrids/.DS_Store differ
diff --git a/node_modules/hybrids/src/cache.js b/node_modules/hybrids/src/cache.js
index b48d197..a7a48c0 100644
--- a/node_modules/hybrids/src/cache.js
+++ b/node_modules/hybrids/src/cache.js
@@ -182,7 +182,7 @@ function deleteEntry(entry) {
   gcList.add(entry);
 }
 
-function invalidateEntry(entry, clearValue, deleteValue) {
+function invalidateEntry(entry, clearValue, deleteValue, opts) {
   entry.depState = 0;
 
   dispatchDeep(entry);
@@ -194,9 +194,13 @@ function invalidateEntry(entry, clearValue, deleteValue) {
   if (deleteValue) {
     deleteEntry(entry);
   }
+
+  if (opts.force) {
+    entry.state += 1;
+  }
 }
 
-export function invalidate(target, key, clearValue, deleteValue) {
+export function invalidate(target, key, clearValue, deleteValue, opts) {
   if (contexts.size) {
     throw Error(
       `Invalidating property in chain of get calls is forbidden: '${key}'`,
@@ -204,7 +208,7 @@ export function invalidate(target, key, clearValue, deleteValue) {
   }
 
   const entry = getEntry(target, key);
-  invalidateEntry(entry, clearValue, deleteValue);
+  invalidateEntry(entry, clearValue, deleteValue, opts);
 }
 
 export function invalidateAll(target, clearValue, deleteValue) {
diff --git a/node_modules/hybrids/src/define.js b/node_modules/hybrids/src/define.js
index d537946..2312095 100644
--- a/node_modules/hybrids/src/define.js
+++ b/node_modules/hybrids/src/define.js
@@ -72,8 +72,8 @@ function compile(Hybrid, descriptors) {
 
     if (config.connect) {
       callbacks.push(host =>
-        config.connect(host, key, () => {
-          cache.invalidate(host, key);
+        config.connect(host, key, (opts) => {
+          cache.invalidate(host, key, false, false, opts);
         }),
       );
     }

The solving PR could also wrap clearEntry and deleteEntry up into the internal options object, rather than using 5 params. User-facing API would still only have force

This issue body was partially generated by patch-package.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

1 participant