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 endless webcam re-render with Golden Retriever #4111

Merged
merged 4 commits into from
Sep 19, 2022

Conversation

Murderlon
Copy link
Member

@Murderlon Murderlon commented Sep 15, 2022

Fixes #4093

During this I shockingly found out about this in the install of Golden Retriever:

    this.uppy.on('state-update', this.saveFilesStateToLocalStorage)

state-update is an undocumented event which triggers on any state change in uppy or in any of the plugin states. This results in this function being called regularly every second. Every call, whenever there are no files (happens a lot), we do a state update, which triggers this function again. This PR fixes the latter problem.

But in my opinion the state-update event shouldn't exist and we should only be able to listen to updates that matter for Golden Retriever. However, that discussion/solution shouldn't have to block this fix.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@Murderlon Murderlon requested review from mifi, arturi and aduh95 September 15, 2022 15:38
Copy link
Contributor

@mifi mifi left a comment

Choose a reason for hiding this comment

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

nice find! so that's why the bug happens yea, because of infinite recursion? .e.g. state-update calls setState, which calls state-update which calls setState and so on.... I wonder why this bug appeared recently.

there is a thottle tough:

this.saveFilesStateToLocalStorage = throttle(

so the event will only be "hammered" twice per second, which might not be too bad (?) but I agree it would make sense to only save the files when they actually change, not when any state changes. maybe listen to file-added, file-removed, file-editor:complete instead.

Verified

This commit was signed with the committer’s verified signature.
crazy-max CrazyMax
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
This reverts commit 81ba3bf.
@Murderlon

This comment was marked as resolved.

@Murderlon Murderlon merged commit 895e96f into main Sep 19, 2022
@Murderlon Murderlon deleted the golden-retriever-state-fix branch September 19, 2022 11:58
Murderlon added a commit that referenced this pull request Sep 19, 2022
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
github-actions bot added a commit that referenced this pull request Sep 25, 2022
| Package                   | Version | Package                   | Version |
| ------------------------- | ------- | ------------------------- | ------- |
| @uppy/angular             |   0.3.2 | @uppy/thumbnail-generator |   2.2.2 |
| @uppy/core                |   2.3.4 | @uppy/robodog             |   2.9.5 |
| @uppy/dashboard           |   2.4.3 | uppy                      |  2.13.6 |
| @uppy/golden-retriever    |   2.1.2 |                           |         |

- @uppy/golden-retriever: Fix endless webcam re-render with Golden Retriever (Merlijn Vos / #4111)
- meta: run CI when modifying workflow files (Antoine du Hamel / #4091)
- meta: limit the number of unnecessary CI runs (Antoine du Hamel / #4086)
- meta: fix typo in `e2e.yml` (Antoine du Hamel)
- meta: Restrict e2e CI runs (Merlijn Vos / #4075)
- @uppy/core: Fix `Restrictor` counts ghost files against `maxNumberOfFiles` (Andrew McIntee / #4078)
- meta: improve CI npm install time (Antoine du Hamel / #4058)
- meta: fix Node.js 12.x CI (Antoine du Hamel)
- @uppy/core: fix types (Antoine du Hamel / #4072)
- @uppy/core,@uppy/dashboard,@uppy/thumbnail-generator: update definition type files for TS 4.8 compatibility (Antoine du Hamel / #4055)
@github-actions github-actions bot mentioned this pull request Sep 25, 2022
github-actions bot added a commit that referenced this pull request Sep 25, 2022
| Package                   | Version | Package                   | Version |
| ------------------------- | ------- | ------------------------- | ------- |
| @uppy/angular             |   0.4.2 | @uppy/onedrive            |   3.0.1 |
| @uppy/audio               |   1.0.2 | @uppy/progress-bar        |   3.0.1 |
| @uppy/aws-s3              |   3.0.2 | @uppy/provider-views      |   3.0.1 |
| @uppy/aws-s3-multipart    |   3.0.2 | @uppy/react               |   3.0.2 |
| @uppy/box                 |   2.0.1 | @uppy/redux-dev-tools     |   3.0.1 |
| @uppy/companion           |   4.0.2 | @uppy/remote-sources      |   1.0.2 |
| @uppy/companion-client    |   3.0.2 | @uppy/screen-capture      |   3.0.1 |
| @uppy/compressor          |   1.0.1 | @uppy/status-bar          |   3.0.1 |
| @uppy/core                |   3.0.2 | @uppy/store-default       |   3.0.2 |
| @uppy/dashboard           |   3.1.0 | @uppy/store-redux         |   3.0.2 |
| @uppy/drag-drop           |   3.0.1 | @uppy/svelte              |   3.0.1 |
| @uppy/drop-target         |   2.0.1 | @uppy/thumbnail-generator |   3.0.2 |
| @uppy/dropbox             |   3.0.1 | @uppy/transloadit         |   3.0.2 |
| @uppy/facebook            |   3.0.1 | @uppy/tus                 |   3.0.2 |
| @uppy/file-input          |   3.0.1 | @uppy/unsplash            |   3.0.1 |
| @uppy/form                |   3.0.1 | @uppy/url                 |   3.0.1 |
| @uppy/golden-retriever    |   3.0.1 | @uppy/utils               |   5.0.2 |
| @uppy/google-drive        |   3.0.1 | @uppy/vue                 |   1.0.1 |
| @uppy/image-editor        |   2.0.1 | @uppy/webcam              |   3.2.0 |
| @uppy/informer            |   3.0.1 | @uppy/xhr-upload          |   3.0.2 |
| @uppy/instagram           |   3.0.1 | @uppy/zoom                |   2.0.1 |
| @uppy/locales             |   3.0.1 | uppy                      |   3.1.0 |

- meta: Fix companion-deploy-yml (Mikael Finstad)
- website: fix tag for Activity Feed (Livia Medeiros / #4118)
- @uppy/golden-retriever: fix condition to load files from service worker (Merlijn Vos / #4115)
- website: remove references to the deleted `disc.html` page (Antoine du Hamel / #4119)
- @uppy/locales: Create uz_UZ (Ozodbek1405 / #4114)
- @uppy/golden-retriever: Fix endless webcam re-render with Golden Retriever (Merlijn Vos / #4111)
- @uppy/image-editor: image-editor: fix controls in small Dashboard (Livia Medeiros / #4113)
- website: add “what is Uppy” to the blog post (Artur Paikin)
- meta: fix Companion deploy (Antoine du Hamel / #4095)
- @uppy/dashboard: add dashboard:show-panel event (Jon-Pierre Sanchez / #4108)
- website: Small post fixes (Artur Paikin)
- @uppy/companion: Companion throttle progress by time (Mikael Finstad / #4101)
- meta: skip a few more unnecessary CI runs (Antoine du Hamel / #4106)
- meta: resolve e2e flakiness (Merlijn Vos / #4077)
- meta: run linters on almost every PRs (Antoine du Hamel / #4105)
- website: 3.0 blog post tweaks (Merlijn Vos / #4104)
- meta: Fix linter warnings in 3.0 post (Murderlon)
- website: Add 3.0 blog post (Artur Paikin / #4046)
- website: fix ESM import in example (Livia Medeiros / #4103)
- doc: Update "Dashboard typo" (Laban / #4096)
- @uppy/audio,@uppy/aws-s3-multipart,@uppy/aws-s3,@uppy/box,@uppy/companion-client,@uppy/companion,@uppy/compressor,@uppy/core,@uppy/dashboard,@uppy/drag-drop,@uppy/drop-target,@uppy/dropbox,@uppy/facebook,@uppy/file-input,@uppy/form,@uppy/golden-retriever,@uppy/google-drive,@uppy/image-editor,@uppy/informer,@uppy/instagram,@uppy/locales,@uppy/onedrive,@uppy/progress-bar,@uppy/provider-views,@uppy/react,@uppy/redux-dev-tools,@uppy/remote-sources,@uppy/screen-capture,@uppy/status-bar,@uppy/store-default,@uppy/store-redux,@uppy/svelte,@uppy/thumbnail-generator,@uppy/transloadit,@uppy/tus,@uppy/unsplash,@uppy/url,@uppy/utils,@uppy/vue,@uppy/webcam,@uppy/xhr-upload,@uppy/zoom: add missing entries to changelog for individual packages (Antoine du Hamel / #4092)
- meta: ci: add GHA to tryout bundling Uppy with popular bundlers (Antoine du Hamel / #4084)
- @uppy/core: Fix `Restrictor` counts ghost files against `maxNumberOfFiles` (Andrew McIntee / #4078)
- uppy: add a decoy `Core` export to warn users about the renaming (Antoine du Hamel / #4085)
- meta: run CI when modifying workflow files (Antoine du Hamel / #4091)
- meta: limit the number of unnecessary CI runs (Antoine du Hamel / #4086)
- meta: Update remote-sources.md (heocoi / #4087)
- uppy: remove all remaining occurrences of `Uppy.Core` (Antoine du Hamel / #4082)
- meta: fix typo in `e2e.yml` (Antoine du Hamel)
- meta: Restrict e2e CI runs (Merlijn Vos / #4075)
- @uppy/webcam: Set default videoConstraints (Artur Paikin / #4070)
- @uppy/angular: Fix angular build error (Murderlon)
- website: add `Known issues` section on Migration Guide (Antoine du Hamel / #4066)
- @uppy/core: fix types (Antoine du Hamel / #4072)
- doc: remove use of deprecated `metaFields` option (Antoine du Hamel / #4073)
HeavenFox pushed a commit to docsend/uppy that referenced this pull request Jun 27, 2023
| Package                   | Version | Package                   | Version |
| ------------------------- | ------- | ------------------------- | ------- |
| @uppy/angular             |   0.4.2 | @uppy/onedrive            |   3.0.1 |
| @uppy/audio               |   1.0.2 | @uppy/progress-bar        |   3.0.1 |
| @uppy/aws-s3              |   3.0.2 | @uppy/provider-views      |   3.0.1 |
| @uppy/aws-s3-multipart    |   3.0.2 | @uppy/react               |   3.0.2 |
| @uppy/box                 |   2.0.1 | @uppy/redux-dev-tools     |   3.0.1 |
| @uppy/companion           |   4.0.2 | @uppy/remote-sources      |   1.0.2 |
| @uppy/companion-client    |   3.0.2 | @uppy/screen-capture      |   3.0.1 |
| @uppy/compressor          |   1.0.1 | @uppy/status-bar          |   3.0.1 |
| @uppy/core                |   3.0.2 | @uppy/store-default       |   3.0.2 |
| @uppy/dashboard           |   3.1.0 | @uppy/store-redux         |   3.0.2 |
| @uppy/drag-drop           |   3.0.1 | @uppy/svelte              |   3.0.1 |
| @uppy/drop-target         |   2.0.1 | @uppy/thumbnail-generator |   3.0.2 |
| @uppy/dropbox             |   3.0.1 | @uppy/transloadit         |   3.0.2 |
| @uppy/facebook            |   3.0.1 | @uppy/tus                 |   3.0.2 |
| @uppy/file-input          |   3.0.1 | @uppy/unsplash            |   3.0.1 |
| @uppy/form                |   3.0.1 | @uppy/url                 |   3.0.1 |
| @uppy/golden-retriever    |   3.0.1 | @uppy/utils               |   5.0.2 |
| @uppy/google-drive        |   3.0.1 | @uppy/vue                 |   1.0.1 |
| @uppy/image-editor        |   2.0.1 | @uppy/webcam              |   3.2.0 |
| @uppy/informer            |   3.0.1 | @uppy/xhr-upload          |   3.0.2 |
| @uppy/instagram           |   3.0.1 | @uppy/zoom                |   2.0.1 |
| @uppy/locales             |   3.0.1 | uppy                      |   3.1.0 |

- meta: Fix companion-deploy-yml (Mikael Finstad)
- website: fix tag for Activity Feed (Livia Medeiros / transloadit#4118)
- @uppy/golden-retriever: fix condition to load files from service worker (Merlijn Vos / transloadit#4115)
- website: remove references to the deleted `disc.html` page (Antoine du Hamel / transloadit#4119)
- @uppy/locales: Create uz_UZ (Ozodbek1405 / transloadit#4114)
- @uppy/golden-retriever: Fix endless webcam re-render with Golden Retriever (Merlijn Vos / transloadit#4111)
- @uppy/image-editor: image-editor: fix controls in small Dashboard (Livia Medeiros / transloadit#4113)
- website: add “what is Uppy” to the blog post (Artur Paikin)
- meta: fix Companion deploy (Antoine du Hamel / transloadit#4095)
- @uppy/dashboard: add dashboard:show-panel event (Jon-Pierre Sanchez / transloadit#4108)
- website: Small post fixes (Artur Paikin)
- @uppy/companion: Companion throttle progress by time (Mikael Finstad / transloadit#4101)
- meta: skip a few more unnecessary CI runs (Antoine du Hamel / transloadit#4106)
- meta: resolve e2e flakiness (Merlijn Vos / transloadit#4077)
- meta: run linters on almost every PRs (Antoine du Hamel / transloadit#4105)
- website: 3.0 blog post tweaks (Merlijn Vos / transloadit#4104)
- meta: Fix linter warnings in 3.0 post (Murderlon)
- website: Add 3.0 blog post (Artur Paikin / transloadit#4046)
- website: fix ESM import in example (Livia Medeiros / transloadit#4103)
- doc: Update "Dashboard typo" (Laban / transloadit#4096)
- @uppy/audio,@uppy/aws-s3-multipart,@uppy/aws-s3,@uppy/box,@uppy/companion-client,@uppy/companion,@uppy/compressor,@uppy/core,@uppy/dashboard,@uppy/drag-drop,@uppy/drop-target,@uppy/dropbox,@uppy/facebook,@uppy/file-input,@uppy/form,@uppy/golden-retriever,@uppy/google-drive,@uppy/image-editor,@uppy/informer,@uppy/instagram,@uppy/locales,@uppy/onedrive,@uppy/progress-bar,@uppy/provider-views,@uppy/react,@uppy/redux-dev-tools,@uppy/remote-sources,@uppy/screen-capture,@uppy/status-bar,@uppy/store-default,@uppy/store-redux,@uppy/svelte,@uppy/thumbnail-generator,@uppy/transloadit,@uppy/tus,@uppy/unsplash,@uppy/url,@uppy/utils,@uppy/vue,@uppy/webcam,@uppy/xhr-upload,@uppy/zoom: add missing entries to changelog for individual packages (Antoine du Hamel / transloadit#4092)
- meta: ci: add GHA to tryout bundling Uppy with popular bundlers (Antoine du Hamel / transloadit#4084)
- @uppy/core: Fix `Restrictor` counts ghost files against `maxNumberOfFiles` (Andrew McIntee / transloadit#4078)
- uppy: add a decoy `Core` export to warn users about the renaming (Antoine du Hamel / transloadit#4085)
- meta: run CI when modifying workflow files (Antoine du Hamel / transloadit#4091)
- meta: limit the number of unnecessary CI runs (Antoine du Hamel / transloadit#4086)
- meta: Update remote-sources.md (heocoi / transloadit#4087)
- uppy: remove all remaining occurrences of `Uppy.Core` (Antoine du Hamel / transloadit#4082)
- meta: fix typo in `e2e.yml` (Antoine du Hamel)
- meta: Restrict e2e CI runs (Merlijn Vos / transloadit#4075)
- @uppy/webcam: Set default videoConstraints (Artur Paikin / transloadit#4070)
- @uppy/angular: Fix angular build error (Murderlon)
- website: add `Known issues` section on Migration Guide (Antoine du Hamel / transloadit#4066)
- @uppy/core: fix types (Antoine du Hamel / transloadit#4072)
- doc: remove use of deprecated `metaFields` option (Antoine du Hamel / transloadit#4073)
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.

Webcam with GoldenRetriever: Video preview reload bug (reproducible on uppy.io site example)
3 participants