Skip to content

Change input after initial render #56

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
Wykks opened this issue Nov 18, 2019 · 9 comments · Fixed by #59
Closed

Change input after initial render #56

Wykks opened this issue Nov 18, 2019 · 9 comments · Fixed by #59
Labels
enhancement New feature or request released

Comments

@Wykks
Copy link

Wykks commented Nov 18, 2019

Hi!

What's the correct way to change input after initial render ?
Because componentProperties only set input on the first render, but we cannot test subsequent input changes this way (which leads to subsequent calls to ngOnChanges).

For now, this works :

component.fixture.componentInstance.myInput = 'stuff';
component.detectChanges();

But accessing fixture is not recommended (https://testing-library.com/docs/angular-testing-library/api#fixture, and I agree with this), and since I do not consider testing dynamic input change is testing an implementation detail, I believe there's should be another way, right ?

Edit: Maybe something like this :

// component is a RenderResult here
component.setComponentInputs({
   myInput: 'value'
});
// And call detectChanges / ngOnChanges automatically

Also, thanks for this library it's really usefull !

@timdeschryver
Copy link
Member

Hi, thanks for the feedback!

When this lib was created I had the opinion that dynamic input change was a bad practice, but now that I read this issue I'm rethinking it.

What do you think the API should look like, something as the following?

// rerender has a componentProperties parameter
rerender({
  myInput: 'foo'
})

Internally, we'll update the properties and run a CD cycle.

@timdeschryver timdeschryver added the enhancement New feature or request label Nov 19, 2019
@Wykks
Copy link
Author

Wykks commented Nov 20, 2019

rerender seems a weird name for something that should "just" change inputs. But I get the idea 😄

Also, thinking more about it, although it would be useful when using a host/wrapper component; without that, we would still need to manually call ngOnChanges afterwards.

Do you think it would be a bad idea to automatically call ngOnChanges here too (with generatedSimpleChanges params) ? (also on render too, but that would be a breaking change...).
I know, "This would be perilous" angular/angular#6235 (comment); but in practice, it's not a change angular would make easily so...

@timdeschryver
Copy link
Member

rerender is taken from the other testing libraries, to keep the API consistent.

We'll first do that, and see what we can do about ngOnChanges afterwards.

@timdeschryver
Copy link
Member

Are you interested in creating this feature @Wykks ?

@timdeschryver
Copy link
Member

🎉 This issue has been resolved in version 8.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@miluoshi
Copy link
Contributor

miluoshi commented May 25, 2020

@timdeschryver ngOnChanges could be called automatically both on the first render and on rerender if a component implements it. Could we implement this into the library?

See this pseudo-code:

import { SimpleChange } from '@angular/core';
import { ComponentFixture } from '@angular/core/testing';
import { render } from '@testing-library/angular';

const setupComponent = async () => {
  const someProps = {};
  
  const { rerender, fixture } = await render(SomeComponent, { componentProperties: someProps });
  
  const getChangesObj = (
    oldProps: Partial<SomeComponent> | null, 
    newProps: Partial<SomeComponent>, 
    isFirstChange: boolean
  ) => {
    return Object.keys(newProps).reduce((changes, key) => ({
      ...changes,
      [key]: new SimpleChange(isFirstChange ? null : oldProps[key], newProps[key], false)
    }), {});
  };
  
  // Run ngOnChanges on initial render
  if ('ngOnChanges' in fixture.componentInstance) {
    fixture.componentInstance.ngOnChanges(getChangesObj(null, someProps, true));
  }
  
  const updatedRerender = (props: Partial<SomeComponent>) => {
    const changes = getChangesObj(fixture.componentInstance, props, false);

    rerender(props);

    if ('ngOnChanges' in fixture.componentInstance) {
      fixture.componentInstance.ngOnChanges(changes);
    }
  }

  return {
    rerender: updatedRerender
  };
}

@timdeschryver
Copy link
Member

@miluoshi do you want to create a PR for this?

@miluoshi
Copy link
Contributor

miluoshi commented Jun 19, 2020

@miluoshi do you want to create a PR for this?

@timdeschryver Sure, here it is 🙂
#110

@timdeschryver
Copy link
Member

🎉 This issue has been resolved in version 9.4.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request released
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants