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

ensure that onScrollListener is actually defined before removing #25

Merged
merged 1 commit into from Mar 23, 2017
Merged

ensure that onScrollListener is actually defined before removing #25

merged 1 commit into from Mar 23, 2017

Conversation

ghost
Copy link

@ghost ghost commented Mar 21, 2017

I had a problem when using VirtualScrollComponent in my application and writing tests. A single test containing the VirtualScrollComponent failed (see below) and as a results all following tests also failed. It was very hard to find out the actual problem but I think I could figure out the original cause.

Repro:

  1. Use VirtualScrollComponent in template of another component e.g. called MyComponent
  2. Write a test for MyComponent that checks whether an error is thrown when something is not initialized properly, e.g.
    it('should throw if no ... is given', () => { expect(() => fixture.detectChanges()).toThrowError(); });
  3. When this test is executed, MyComponent is not created properly, therefore I assume that the listener is not attached properly either. However the test cleanup may call destroy of the VirtualScrollComponent internally anyways.
  4. As this.onScrollListener has not been defined, the cleanup phase failed causing all subsequent test to fail, too.

-> I could fix this by ensuring that onScrollListener is not undefined before removing the listener.

@rintoj rintoj merged commit b9c377f into rintoj:master Mar 23, 2017
@rintoj
Copy link
Owner

rintoj commented Mar 23, 2017

@davidwuerfel, thank you.

@rintoj
Copy link
Owner

rintoj commented Mar 23, 2017

@davidwuerfel, you are changes are published with v0.1.4 and is now available in npm repo.

rintoj pushed a commit that referenced this pull request Mar 23, 2017
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.

1 participant