proposal: Cancel assemblies optional #3575

mifi · 2022-03-16T05:20:13Z

to make canceling assemblies optional
possibly fixes #3547
note: not tested

to make canceling assemblies optional possibly fixes #3547

Murderlon

I don't think this is the way to do it as it goes against the separation of concerns. Core doesn't know about the concept of assemblies, only @uppy/transloadit does, therefor this does not belong here.

In my opinion if the problem is framework or transloadit specific, the problem should be solved there, or an agnostic solution.

mifi · 2022-03-16T05:35:25Z

I think I agree with that. But I'm not sure how to solve it. If we renamed cancelAssemblies: true to userCanceled: true and cancel-all-assemblies to user-cancel-all, then core would no longer have to deal with the concept of assemblies, right?

I think in one way or another, core would need to distinguish between the two cases:

the user cancels all
the react hook cancels all

Murderlon · 2022-03-16T05:54:05Z

I think in one way or another, core would need to distinguish between the two cases:

the user cancels all

the react hook cancels all

This fact that it says that core should know the difference between a react hook cancel and another cancel is still problematic to me, for the same argument.

So far the most reasonable solution to me is adding { reason: 'user' } to the cancel all call on the button. Then add an if statement in @uppy/transloadit and leave it at that.

mifi · 2022-03-16T05:58:55Z

By adding a { reason: 'user' }, core also needs to know the difference between two cancel types: one is user cancel and other is non-user, which in our use case is hook cancel (are there other use cases?). We would need to make reason default to user then, to make it backwards compatible, right?

mifi · 2022-03-16T06:12:22Z

Ok I rewrote to reason='user' logic. still untested

Murderlon · 2022-03-17T09:39:18Z

packages/@uppy/react/src/useUppy.js

@@ -17,7 +17,7 @@ module.exports = function useUppy (factory) {

  useEffect(() => {
    return () => {
-      uppy.current.close()
+      uppy.current.close({ reason: null })


Explicitly passing null still feels a bit off to me. You would expect a nullish value to be the default.

True. Which reason should we use? cleanup ?

Cleanup also sounds inherit to close in itself. In the end it is the difference of human/user vs. machine. So perhaps reason: 'unmount' does make sense after all.

Wouldn't we then bring a concept of react into core, which would break the separation of concerns?

Has nothing to do with React per se, you can unmount in vanilla JS (remove it programmatically), vue, svelte, angular, whatever.

Murderlon

If the tests pass, ideally with a new test for this change, then we are good to go.

mifi · 2022-03-21T04:35:46Z

yes and documentation. not sure why the end to end tests failed though. need to test locally to see if I broke something

Murderlon · 2022-03-21T04:41:40Z

Try merging latest main into this branch. If it isn't fixed than I can change some e2e code.

website/src/docs/core.md

packages/@uppy/core/src/Uppy.js

Murderlon · 2022-03-22T06:24:22Z

website/src/docs/core.md


 Uninstall all plugins and close down this Uppy instance. Also runs `uppy.reset()` before uninstalling.

+* `reason`: If set to the string `user`, it will also cancel any running Transloadit assemblies. Set to `unmount` to disable this behavior.


This should not be documented in core. Instead, we should explain the possible values of reason and that plugins can use that for wanted behavior.

I updated the docs

Murderlon · 2022-03-22T06:25:57Z

Can you also check whether the same is needed in Vue, Svelte, and Angular? We also need a test for this.

also fix rule of hooks issue

mifi · 2022-03-22T07:03:37Z

couldn't find close being called in Vue, Svelte, and Angular, however I'm not very proficient in any of those so I may be missing something. I updated a lot of example code though, to include reason: 'unmount' and some tests

Murderlon · 2022-03-22T08:25:53Z

website/src/docs/core.md

+  * `user` - User has explicitly closed the Uppy and any ongoing Uploads
+  * `unmount` - Uppy has been unmounted (removed from the user interface)


Suggested change

* `user` - User has explicitly closed the Uppy and any ongoing Uploads

* `unmount` - Uppy has been unmounted (removed from the user interface)

* `user` - The user has closed the Uppy instance

* `unmount` - The uppy instance has been closed programatically

Murderlon · 2022-03-22T08:26:34Z

website/src/docs/core.md


 Stop all uploads in progress and clear file selection, set progress to 0. More or less, it returns things to the way they were before any user input.

-### `uppy.close()`
+### `uppy.close({ reason = 'user' })`


Shouldn't this also be added for uppy.cancelAll?

website/src/docs/transloadit.md

mifi · 2022-03-22T14:04:42Z

e2e tests failed with

     AssertionError: Timed out retrying after 4000ms: Expected to find element: `.uppy-StatusBar-statusPrimary`, but never found it.

is it a flaky test again?

mifi · 2022-03-22T14:35:49Z

Tests re-ran successfully now, so I think it's indeed a flaky test @Murderlon

arturi · 2022-03-22T16:28:41Z

Are we changing the behaviour for Companion remote uploads as well? So that pressing “cancel” (reason: 'user') will cancel remote uploads too?

mifi · 2022-03-23T14:20:59Z

I haven't changed that behavior. Not sure where to change it

mifi · 2022-03-28T05:34:49Z

Ok I have now implemented the reason === 'user' check also for (hopefully) all upload variants, so that upload will continue even though an unmount happens.

I also had to prevent the file-removed event from being emitted when cancelAll is called with reason === 'user'.
Not 100% sure if this is the right way to implement it but it seems to work.

Co-authored-by: Merlijn Vos <merlijn@soverin.net>

mifi · 2022-03-28T05:54:21Z

Tests passed! I'm not sure if this is considered a breaking change now. If closing Uppy (after this PR) with reason unmount and there are uploads still ongoing, the closed uppy instance will still continue to emit events event after it's closed, like progress and success.

Murderlon · 2022-03-28T06:14:21Z

Under semver this is not a breaking change and I think conceptually it's acceptable too. It won't send events endlessly, the upload will fail or succeed and it will stop.

arturi · 2022-03-28T12:29:07Z

Awesome! I wonder if this will change this #2301, but you’d still get the removed-by-user when individual file is removed, right?

uppy.on('file-removed', (file, reason) => {
  // reasons: `removed-by-user` and `cancel-all`
  removeFileFromUploadingCounterUI(file)
  if (reason === 'removed-by-user') {
    sendDeleteRequestForFile(file)
  }
})

mifi · 2022-03-28T14:40:50Z

I think so. Didn't change that logic.

mifi · 2022-04-11T06:46:55Z

I think this can be merged @arturi ?

Murderlon · 2022-04-12T10:45:28Z

You can go ahead and merge it when you're ready :)

# Conflicts: # packages/@uppy/react/src/useUppy.test.js

| Package | Version | Package | Version | | ---------------------- | ------- | ---------------------- | ------- | | @uppy/audio | 0.3.1 | @uppy/provider-views | 2.1.0 | | @uppy/aws-s3 | 2.1.0 | @uppy/react | 2.2.0 | | @uppy/aws-s3-multipart | 2.3.0 | @uppy/react-native | 0.3.0 | | @uppy/companion-client | 2.1.0 | @uppy/screen-capture | 2.1.0 | | @uppy/core | 2.2.0 | @uppy/status-bar | 2.2.0 | | @uppy/dashboard | 2.2.0 | @uppy/svelte | 1.0.8 | | @uppy/drag-drop | 2.1.0 | @uppy/transloadit | 2.2.0 | | @uppy/file-input | 2.1.0 | @uppy/tus | 2.3.0 | | @uppy/google-drive | 2.1.0 | @uppy/url | 2.1.0 | | @uppy/image-editor | 1.2.0 | @uppy/webcam | 2.2.0 | | @uppy/instagram | 2.1.0 | @uppy/xhr-upload | 2.1.0 | | @uppy/locales | 2.1.0 | @uppy/zoom | 1.1.0 | | @uppy/onedrive | 2.1.0 | @uppy/robodog | 2.6.0 | | @uppy/progress-bar | 2.1.0 | uppy | 2.10.0 | - @uppy/audio: fix types (Merlijn Vos / #3689) - @uppy/aws-s3-multipart,@uppy/aws-s3,@uppy/core,@uppy/react,@uppy/transloadit,@uppy/tus,@uppy/xhr-upload: proposal: Cancel assemblies optional (Mikael Finstad / #3575) - @uppy/aws-s3-multipart: export interface AwsS3MultipartOptions (Matteo Padovano / #3709) - @uppy/companion-client: refactor to ESM (Antoine du Hamel / #3693) - @uppy/companion: Only deploy on companion changes (kiloreux / #3677) - @uppy/core: add definition for addFiles method (Matteo Padovano / #3556) - @uppy/core: wrap plugins in div.uppy-Root and set dir attrubute in UIPlugin (Artur Paikin / #3692) - @uppy/google-drive: refactor to ESM (Antoine du Hamel / #3683) - @uppy/image-editor: refactor to ESM (Antoine du Hamel / #3685) - @uppy/instagram: refactor to ESM (Antoine du Hamel / #3696) - @uppy/locales: Add `save` translation to Spanish locale (Juan Carlos Alonso / #3678) - @uppy/locales: refactor to ESM (Antoine du Hamel / #3707) - @uppy/onedrive: refactor to ESM (Antoine du Hamel / #3694) - @uppy/progress-bar: refactor to ESM (Antoine du Hamel / #3706) - @uppy/provider-views: refactor to ESM (Antoine du Hamel / #3715) - @uppy/react: Support React 18 in @uppy/react (Merlijn Vos / #3680) - @uppy/screen-capture: refactor to ESM (Antoine du Hamel / #3698) - @uppy/status-bar: refactor to ESM (Antoine du Hamel / #3697) - @uppy/transloadit: add rate limiting for assembly creation and status polling (Antoine du Hamel / #3718) - @uppy/tus: refactor to ESM (Antoine du Hamel / #3724) - @uppy/url: refactor to ESM (Antoine du Hamel / #3713) - @uppy/webcam: refactor to ESM (Antoine du Hamel / #3686) - @uppy/xhr-upload: refactor to ESM (Antoine du Hamel / #3695) - @uppy/zoom: refactor to ESM (Antoine du Hamel / #3699) - meta: e2e: fix failing test (Antoine du Hamel / #3722) - test: harden linter rule for JSX/ESM validation (Antoine du Hamel / #3681) - test: harden linter rules for ESM/CJS validation (Antoine du Hamel / #3674) - test: Increase retries to trigger longer retryDelay in tus (Artur Paikin / #3726) - test: Remove `it.only` from e2e test (Merlijn Vos / #3690) - tests: Make Cypress more stable & add e2e test for error events when upload fails (Merlijn Vos / #3662)

mifi added 2 commits March 16, 2022 13:17

change cancel logic

1775410

to make canceling assemblies optional possibly fixes #3547

add forgotten file

808ade1

Murderlon requested changes Mar 16, 2022

View reviewed changes

rewrite to reason='user'

1b90fbd

try to fix crash

551915d

arturi self-requested a review March 16, 2022 13:33

Murderlon reviewed Mar 17, 2022

View reviewed changes

change reason to unmount

94bc058

Murderlon reviewed Mar 21, 2022

View reviewed changes

Merge branch 'main' into cancel-assemblies-optional

aff8523

mifi commented Mar 22, 2022

View reviewed changes

website/src/docs/core.md Outdated Show resolved Hide resolved

packages/@uppy/core/src/Uppy.js Outdated Show resolved Hide resolved

Apply suggestions from code review

df9acfd

Murderlon reviewed Mar 22, 2022

View reviewed changes

mifi added 3 commits March 22, 2022 14:44

add close+unmount in more code examples

b251118

also fix rule of hooks issue

improve reason docs

c6a297a

add tests

5aaa240

Murderlon reviewed Mar 22, 2022

View reviewed changes

add options also to reset

49ae0f3

update doc

5141d93

also prevent canceling uploads on unmount

be3088a

Update website/src/docs/transloadit.md

38c9ed3

Co-authored-by: Merlijn Vos <merlijn@soverin.net>

Murderlon approved these changes Mar 28, 2022

View reviewed changes

Murderlon added the pending end-to-end tests label Mar 28, 2022

github-actions bot removed the pending end-to-end tests label Mar 28, 2022

Murderlon added the safe to test Add this label on trustworthy PRs to spawn the e2e test suite label Mar 28, 2022

github-actions bot removed the safe to test Add this label on trustworthy PRs to spawn the e2e test suite label Mar 28, 2022

arturi approved these changes Apr 28, 2022

View reviewed changes

mifi added 2 commits May 5, 2022 10:59

Merge branch 'main' into cancel-assemblies-optional

834cc1f

# Conflicts: # packages/@uppy/react/src/useUppy.test.js

remove conflicting file

c82d0c8

mifi merged commit 8ed2105 into main May 5, 2022

mifi deleted the cancel-assemblies-optional branch May 5, 2022 18:53

github-actions bot mentioned this pull request May 14, 2022

Release: uppy@2.10.0 #3727

Merged

mifi mentioned this pull request Nov 21, 2022

@uppy/react: deprecate useUppy #4223

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

proposal: Cancel assemblies optional #3575

proposal: Cancel assemblies optional #3575

mifi commented Mar 16, 2022

Murderlon left a comment •

edited

Loading

mifi commented Mar 16, 2022 •

edited

Loading

Murderlon commented Mar 16, 2022

mifi commented Mar 16, 2022

mifi commented Mar 16, 2022

Murderlon Mar 17, 2022

mifi Mar 17, 2022

Murderlon Mar 18, 2022

mifi Mar 18, 2022

Murderlon Mar 18, 2022

Murderlon left a comment

mifi commented Mar 21, 2022

Murderlon commented Mar 21, 2022

Murderlon Mar 22, 2022 •

edited

Loading

mifi Mar 22, 2022

Murderlon commented Mar 22, 2022

mifi commented Mar 22, 2022

Murderlon Mar 22, 2022

Murderlon Mar 22, 2022

mifi Mar 22, 2022

mifi commented Mar 22, 2022

mifi commented Mar 22, 2022

arturi commented Mar 22, 2022

mifi commented Mar 23, 2022

mifi commented Mar 28, 2022

mifi commented Mar 28, 2022

Murderlon commented Mar 28, 2022

arturi commented Mar 28, 2022

mifi commented Mar 28, 2022

mifi commented Apr 11, 2022

Murderlon commented Apr 12, 2022 •

edited

Loading


		Uninstall all plugins and close down this Uppy instance. Also runs `uppy.reset()` before uninstalling.

		* `reason`: If set to the string `user`, it will also cancel any running Transloadit assemblies. Set to `unmount` to disable this behavior.

		* `user` - User has explicitly closed the Uppy and any ongoing Uploads
		* `unmount` - Uppy has been unmounted (removed from the user interface)

proposal: Cancel assemblies optional #3575

proposal: Cancel assemblies optional #3575

Conversation

mifi commented Mar 16, 2022

Murderlon left a comment • edited Loading

Choose a reason for hiding this comment

mifi commented Mar 16, 2022 • edited Loading

Murderlon commented Mar 16, 2022

mifi commented Mar 16, 2022

mifi commented Mar 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Murderlon left a comment

Choose a reason for hiding this comment

mifi commented Mar 21, 2022

Murderlon commented Mar 21, 2022

Murderlon Mar 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Murderlon commented Mar 22, 2022

mifi commented Mar 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mifi commented Mar 22, 2022

mifi commented Mar 22, 2022

arturi commented Mar 22, 2022

mifi commented Mar 23, 2022

mifi commented Mar 28, 2022

mifi commented Mar 28, 2022

Murderlon commented Mar 28, 2022

arturi commented Mar 28, 2022

mifi commented Mar 28, 2022

mifi commented Apr 11, 2022

Murderlon commented Apr 12, 2022 • edited Loading

Murderlon left a comment •

edited

Loading

mifi commented Mar 16, 2022 •

edited

Loading

Murderlon Mar 22, 2022 •

edited

Loading

Murderlon commented Apr 12, 2022 •

edited

Loading