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

SpectatorWithHost resetting inputs when detectChanges is set to true #31

Closed
bengry opened this issue Jun 26, 2018 · 21 comments
Closed

SpectatorWithHost resetting inputs when detectChanges is set to true #31

bengry opened this issue Jun 26, 2018 · 21 comments

Comments

@bengry
Copy link

bengry commented Jun 26, 2018

Given the following component:

@Component({
	selector: 'my-component',
    template: `
      <div>
        <ng-container [ngTemplateOutlet]="content"></ng-content>
      </div>`,
	changeDetection: ChangeDetectionStrategy.OnPush,
	encapsulation: ViewEncapsulation.None,
})
export class MyComponent {
  @Input() someInput: boolean;
  @Input() content: TemplateRef;
}

And the following test code:

let spectator: SpectatorWithHost<MyComponent>;
const createHost = createHostComponentFactory({
  component: MyComponent,
  shallow: true
});

it('input test', () => {
  spectator = createHost(`
    <ng-template #content>
      <span>Sample content</span>
    </ng-template>

    <my-component
        [someInput]="someInput"
        [content]="content">
    </my-component>
`, true, { someInput: true })

  expect(spectator.component.someInput).toBe(true);
});

The test 'input test' fails, and someInput is undefined.

passing false as the detectChanges parameter (to createHost) results in someInput being true, but as soon as you run a changeDetection (via spectator.detectChanges() to check the DOM, the values reset.

@NetanelBasal
Copy link
Member

NetanelBasal commented Jun 26, 2018

I'm not sure this is the problem. As you can see here, it should work.
https:/NetanelBasal/spectator/blob/master/src/lib/src/host.ts#L103

Can you create a demo so that I can investigate more, please?
https://stackblitz.com/edit/spectator?file=app%2Fhello.component.spec.ts

@bengry
Copy link
Author

bengry commented Jun 26, 2018

@NetanelBasal I debugged this a bit, and it seems like this line is causing the issue (i.e. the inputs are set fine before it).

You can find a repro for the complexInputs issue, forked from the StackBlitz you linked here: https://stackblitz.com/edit/spectator-9gg3yx?file=app/hello.component.spec.ts

@NetanelBasal
Copy link
Member

can you move this line to 101 and check, please?

@NetanelBasal
Copy link
Member

spectatorWithHost.hostFixture.detectChanges();

@NetanelBasal
Copy link
Member

    if(detectChanges) {
      spectatorWithHost.hostFixture.detectChanges();
    }
    
    if (initialInputs) {
      spectatorWithHost.setInput(initialInputs);
    }

    if (detectChanges) {
      /** Detect changes on the component - useful for onPushComponents */
      spectatorWithHost.detectComponentChanges();
    }

@bengry
Copy link
Author

bengry commented Jun 26, 2018

@NetanelBasal Seems like that works, but I'm not sure how you can run changeDetection before you set the inputs - after all, you want the detection to run on the component with the changes.

@NetanelBasal
Copy link
Member

Yes, but the actual component is:
spectatorWithHost.detectComponentChanges();

You moved the line of the host.

@bengry
Copy link
Author

bengry commented Jun 26, 2018

@NetanelBasal I actually just checked this again against a real component I have and it seems like there's still some issue in this regard. I'll try and make a more exact repro on StackBlitz and add it when done.

@NetanelBasal
Copy link
Member

Already publish 1.8.1 with the fix. Let me know.

@bengry
Copy link
Author

bengry commented Jun 26, 2018

@NetanelBasal FYI there's already a v1.11. Why not 1.11.1?

@NetanelBasal
Copy link
Member

I am using an automatic tool, and it was doing some crazy things.

@NetanelBasal
Copy link
Member

we will be aligned in v2.0

@bengry
Copy link
Author

bengry commented Jun 26, 2018

So 1.8.1 includes everything that 1.11 includes? At any rate, I suggest publishing a 1.11.1 (even if it's === 1.8.1), since no one will see the new fix otherwise (until the next version), and if you install 1.8.1, upgrading to an actual "lower" version is easily possible.

@NetanelBasal This actually just bit us at work, we had ~1.8 on spectator in the package.json, and used spectator.getDirectiveInstance in our tests.
It auto-upgraded to 1.8.1, where getDirectiveInstance is not longer present.

@NetanelBasal
Copy link
Member

Let me check.

@NetanelBasal
Copy link
Member

I need to revert the whole thing; there was a mistake.

@bengry
Copy link
Author

bengry commented Jun 26, 2018

You can't unpublish a package on npm, just deprecate it.

Please post an update when a new version's published, I'll update our projects.

@NetanelBasal
Copy link
Member

Why are you not upgrading to 1.11?

@NetanelBasal
Copy link
Member

OK, so published 1.11.1 as it supposed to be and deprecated 1.8.1. Excuse me for the mess.

@bengry
Copy link
Author

bengry commented Jun 26, 2018

I upgraded to 1.11 originally, but it had the bug I faced that caused 1.8.1 to be released.
That's why I wanted 1.11.1 :-).

I'll give it a try tomorrow. Thanks for the hard work!

@NetanelBasal
Copy link
Member

Thank you.

@NetanelBasal
Copy link
Member

This change broke some of our tests.

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

No branches or pull requests

2 participants