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

Angular: Fix multiple calls of Input setter #17633

Merged
merged 1 commit into from Mar 28, 2022
Merged

Conversation

ThibaudAV
Copy link
Contributor

@ThibaudAV ThibaudAV commented Mar 5, 2022

Maybe : #15610

Potential link : #11613

What I did

Investigate why in some case we have multiple call with some Input setter.

Reasons :
When a Story does not define a template and lets storybook build it from the component
or
When Meta define component and this component is used in the story template

the props are set twice. One for the wrapper component. And also directly set into the target component.
Now only none Input / Output are directly send to the target component
The values for the normal Input will be sent by the angular property biding

How to test

  • Is this testable with Jest or Chromatic screenshots?
  • Does this need a new example in the kitchen sink apps?
  • Does this need an update to the documentation?

If your answer is yes to any of these, please make sure to include it in your PR.

@ThibaudAV ThibaudAV added bug patch:yes Bugfix & documentation PR that need to be picked to main branch labels Mar 5, 2022
@nx-cloud
Copy link

nx-cloud bot commented Mar 5, 2022

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 185b5e7. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@ThibaudAV ThibaudAV force-pushed the angular/fix-setter branch 3 times, most recently from ff94ff2 to 08ab5d0 Compare March 5, 2022 13:55
@@ -104,13 +112,15 @@ describe('StorybookModule', () => {
it('should change inputs if storyProps$ Subject emit', async () => {
const initialProps = {
input: 'input',
inputBindingPropertyName: '',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you are wondering why I am modifying the test here :

If a new entry was added to the list of props between the initialisation of the story and a lazy (emit on storyProps$) change of the input values
like : Initial { InputA : 'A' } new change : { InputA : 'A', InputB: 'B' }
The new Input was not added in the template like the first one <foo [InputA]="InputA" ></foo> because not new rendering a made only rxjs event with new props.
Before it worked because the property was added as a non Input Output property, overwriting the component property directly.
which was not good because angular could not detect the changes with the NgOnChanges for example

But anyway all this should not happen because a higher level mechanism detect if lazy rendering can be done. By comparing templates to see if there is a diff.

so in the test here all Input Output must be existing at init step

@ThibaudAV ThibaudAV marked this pull request as ready for review March 5, 2022 14:18
@ThibaudAV ThibaudAV requested a review from a team March 5, 2022 14:18
@yannbf
Copy link
Member

yannbf commented Mar 7, 2022

Hey @ThibaudAV could you give me a small set of steps to reproduce this in the monorepo? I'd be happy to do some QA!

@ThibaudAV
Copy link
Contributor Author

@yannbf Of course with pleasure :
I did my tests with https://github.com/ThibaudAV/sb-angular13/tree/debug-setter to isolate the bad behaviour.

Without this change

  • Run angular like IRL with yarn ng serve
  • Add something inside html input
  • See in console the log with setter ... are called only once before ngOnChanges
    vs
  • Run Storybook like yarn storybook
  • Add something inside action -> setter
  • See in console the log with setter ... are called one before and one after onChange

So with this change now setter are no more called after ngOnChnages

I will add some specific use case into angular-cli to avoid future regression on this. if you agree ?

@ThibaudAV
Copy link
Contributor Author

👋 @yannbf any news on this ?

Copy link
Member

@yannbf yannbf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @ThibaudAV sorry it took me time to test this. Works closer to the angular app now so it's great 👌
Thanks a lot for the contribution!

@shilman shilman changed the title Angular: Fix multiple call of Input setter Angular: Fix multiple calls of Input setter Mar 28, 2022
@shilman shilman merged commit f09053c into next Mar 28, 2022
@shilman shilman deleted the angular/fix-setter branch March 28, 2022 14:48
@shilman shilman added the patch:done Patch/release PRs already cherry-picked to main/release branch label Apr 1, 2022
shilman added a commit that referenced this pull request Apr 1, 2022
Angular: Fix multiple calls of Input setter
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug patch:done Patch/release PRs already cherry-picked to main/release branch patch:yes Bugfix & documentation PR that need to be picked to main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants