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

bug: Type error when running tests for component with onChange listener #111

Open
3 tasks done
samshareski opened this issue Jun 27, 2022 · 12 comments
Open
3 tasks done
Labels
Resolution: Refine This PR is marked for Jira refinement. We're not working on it - we're talking it through.

Comments

@samshareski
Copy link

samshareski commented Jun 27, 2022

Prerequisites

Stencil Store Version

2.0.0

Stencil Version

2.13.0

Current Behavior

When a component has an onChange listener which has a function that manipulates a https://github.com/State property:

import { onChange } from '../a-store';

export class AppProfile { 
  @State() something = 0;

  componentWillLoad() {
    onChange('clicks', (value) => {
      this.something++;
    });
  }
// ...
}

when I try to unit test the component I get a type error like:

● app-profile › clicks the button again

TypeError: Cannot read properties of undefined (reading '$instanceValues$')

  12 |   componentWillLoad() {
  13 |     onChange('clicks', (value) => {
> 14 |       this.something++;
     |       ^
  15 |     });
  16 |   }
  17 |   

  at getValue (node_modules/@stencil/core/internal/testing/index.js:538:282)
  at AppProfile.get [as something] (node_modules/@stencil/core/internal/testing/index.js:565:13)
  at src/components/app-profile/app-profile.tsx:14:7
  at node_modules/@stencil/store/dist/index.js:144:43
  at node_modules/@stencil/store/dist/index.js:88:40
      at Array.forEach (<anonymous>)
  at reset (node_modules/@stencil/store/dist/index.js:88:24)
  at Object.dispose (node_modules/@stencil/store/dist/index.js:94:9)
  at Object.<anonymous> (src/components/app-profile/app-profile.spec.ts:7:5)

The error only occurs if I'm referencing a @State property, if I reference a component method that just logs something, for example, there is no error.

However, this error doesn't happen with the first test in the suite, only subsequent tests. I'm calling store.dispose() before each test.

Expected Behavior

Tests should run without throwing an error and failing.

Steps to Reproduce

Download repo, run npm install then npm run test

OR

  1. Create a component which registers an onChange listener that references a @State variable

  2. Write a unit test suite for the component with at least 2 tests

  3. Run the unit tests

Code Reproduction URL

https://github.com/samshareski/store-test-bug

Additional Information

No response

@ionitron-bot ionitron-bot bot added the triage label Jun 27, 2022
@alicewriteswrongs
Copy link
Member

Hello @samshareski! Thanks for filing this issue and for providing a helpful reproduction. I can confirm that the bug you described is happening, and after a bit of investigation I'm not sure what the source of the problem is but I believe there may be a bug in stencil-store's logic w/ the onChange handler, with variable scope and this binding, or something along those lines.

I'm going to label this for further investigation and ingestion into our backlog.

Thanks again for reporting!

@alicewriteswrongs alicewriteswrongs added the Resolution: Refine This PR is marked for Jira refinement. We're not working on it - we're talking it through. label Jul 8, 2022
@ionitron-bot ionitron-bot bot removed the triage label Jul 8, 2022
@Serabe
Copy link
Contributor

Serabe commented Jul 12, 2022

@samshareski are you using store.dispose() between tests?

@samshareski
Copy link
Author

@samshareski are you using store.dispose() between tests?

yes

@Serabe
Copy link
Contributor

Serabe commented Jul 12, 2022

Do you have a repo with a reproduction of this issue I can take a look into?

@samshareski
Copy link
Author

Do you have a repo with a reproduction of this issue I can take a look into?

it's linked in the first comment

@samshareski
Copy link
Author

@Serabe
Copy link
Contributor

Serabe commented Jul 12, 2022

I see what's going on. Handlers are never disposed, you need to unregister them in your component. onChange returns a function that when called will unregister everything.

@samshareski
Copy link
Author

@Serabe
samshareski/store-test-bug@24be6fe
is this how that would be set up?

this version still has the test failures occurring

@Serabe
Copy link
Contributor

Serabe commented Jul 12, 2022

Yes. I'll need to investigate then.

@ajgagnon
Copy link

ajgagnon commented Apr 5, 2023

@Serabe I believe this is a bug in stencil itself, so I believe this issue can be closed and cross referenced in stencil:

ionic-team/stencil#4053

As a workaround, in your spec tests, you can manually call disconnectedCallback:

page.rootInstance.disconnectedCallback();

@christophsaile
Copy link

Hey @ajgagnon, I tried calling page.rootInstance.disconnectedCallback(); in my jest test like this:

afterEach(() => {
    page.rootInstance.disconnectedCallback();
});

unfortunatly I get an TypeError: page.rootInstance.disconnectedCallback is not a function after each run. Do you maybe have a working example?

@ajgagnon
Copy link

ajgagnon commented Sep 7, 2023

@christophsaile This is probably because you do not have a disconnectedCallback in your component. You need to add this method to your component:

private removeListener: any;

componentWillLoad() {
    this.removeListener = onChange('clicks', (value) => {
      this.something++;
    });
 }

disconnectedCallback() {
   this.removeListener();
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: Refine This PR is marked for Jira refinement. We're not working on it - we're talking it through.
Projects
None yet
Development

No branches or pull requests

5 participants