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

Fix performance of Angular rerender #6094

Merged
merged 1 commit into from
Mar 19, 2019

Conversation

jonrimmer
Copy link
Contributor

@jonrimmer jonrimmer commented Mar 14, 2019

Issue: #4098

What I did

  • Rewrote forced render handling for Angular to not recreate the whole app, but instead send the updated props to the application using RxJS, then trigger change detection.
  • Added a disableDebounce option to knobs so you can disable the 350ms debounce if you know the re-rendering of the framework you're using isn't broken.

How to test

Run the Angular example and use the knobs. Observe that they update immediately and without lag.

@codecov
Copy link

codecov bot commented Mar 14, 2019

Codecov Report

Merging #6094 into next will decrease coverage by 0.56%.
The diff coverage is 20.68%.

Impacted file tree graph

@@            Coverage Diff             @@
##             next    #6094      +/-   ##
==========================================
- Coverage   37.64%   37.08%   -0.57%     
==========================================
  Files         642      648       +6     
  Lines        9387     9538     +151     
  Branches     1363     1384      +21     
==========================================
+ Hits         3534     3537       +3     
- Misses       5293     5421     +128     
- Partials      560      580      +20
Impacted Files Coverage Δ
app/angular/src/client/preview/render.js 0% <0%> (ø) ⬆️
addons/knobs/src/registerKnobs.js 0% <0%> (ø) ⬆️
...client/preview/angular/components/app.component.ts 25% <20%> (-2.03%) ⬇️
app/angular/src/client/preview/angular/helpers.ts 38.77% <37.5%> (+0.13%) ⬆️
lib/ui/src/core/init-provider-api.js 0% <0%> (ø) ⬆️
lib/cli/lib/initiate.js 0% <0%> (ø) ⬆️
lib/ui/src/provider.js 0% <0%> (ø) ⬆️
lib/ui/src/containers/preview.js 66.66% <0%> (ø) ⬆️
lib/ui/src/core/stories.js 87.5% <0%> (ø) ⬆️
lib/cli/bin/generate.js 0% <0%> (ø) ⬆️
... and 25 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7fbb406...e651a3b. Read the comment docs.

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

@jonrimmer Thanks for this---looks like an awesome solution to a thorny problem! I'm not familiar with Angular enough to do a review, hoping @igor-dv @kroeder @CodeByAlex can take a look.

My two suggestions:

  • Use addParameters({knobs: {disableDebounce: true}} instead of withKnobs({ disableDebounce: true }) => we're trying to migrate everything over to parameters across the board
  • I think the knobs debouncing by @CodeByAlex was mostly for angular. If this PR solves the problem for angular, can we also remove debouncing in the knobs addon?

@shilman shilman added this to the 5.0.x milestone Mar 15, 2019
@jonrimmer
Copy link
Contributor Author

@shilman No problem, I'll update it as you suggest.

Centralised debouncing should be removable if each framework's app implements forced re-render in a way that's sympathetic to its method of receiving updates and updating the DOM. For React and React-esque frameworks that should already be the case, and this PR fixes the situation for Angular.

What I'm not clear about is what the situation is like for other frameworks like Polymer, Vue, etc. It certainly seems like Vue was affected prior to the debounce being added: #6005

@ndelangen
Copy link
Member

@jonrimmer I invited you to the GitHub organisation so you con contribute more easily 👍

@ndelangen
Copy link
Member

Looks good to me, let's get a second opinion out of @kroeder before merging

@jonrimmer
Copy link
Contributor Author

@ndelangen Thanks, I really appreciate it.

@shilman
Copy link
Member

shilman commented Mar 15, 2019

@jonrimmer Good call on making sure Vue works properly, so we can potentially remove global debouncing separately. Super happy with this solution for Angular tho--great work!

@CodeByAlex
Copy link
Member

CodeByAlex commented Mar 15, 2019

@shilman @jonrimmer LGTM. I like how the changes to allow for better updating and rendering are located within the Angular project rather than having to add an "addon-knobs/angular" option which was mentioned in the original issue. Nicely done:)

@backbone87
Copy link
Contributor

since the 4.2 alpha introduced my changes to the storybook vue, debouncing the knobs should not be necessary at all for vue, since we properly change the props of the components in a reactive "vue way".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants