React storage/cancel upload from process file #5179

raystorm · 2024-04-24T21:22:58Z

Description of changes

Adds support for the ability to cancel an Upload from processFile()
by returning a rejected promise.

Issue #, if available

#5099

Description of how you validated changes

Added a Unit Test to ensure removeUpload() is called when a rejected promise is returned from processFile().
Added a New Example, that forces a Promise.reject() in examples.
ran the example manually with additional Logging code
(not committed, and now removed to watch/verify operation.)

Checklist

Have read the Pull Request Guidelines
PR description included
Relevant documentation is changed or added (and PR referenced)
yarn test passes and tests are updated/added
No side effects or sideEffects field updated

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

+ add note about deleteing yarn.lock, so Cypress will download/install correctly + add `nvm install` & `nvm use` to react file.

changeset-bot · 2024-04-24T21:23:02Z

🦋 Changeset detected

Latest commit: af1f328

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@aws-amplify/ui-react-storage	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

calebpollman · 2024-04-24T21:32:56Z

@raystorm Thanks for opening this! Need to discuss with the team, will update here once we have next steps

add changeset

…l-upload-from-processFile # Conflicts: # yarn.lock

calebpollman · 2024-04-26T17:28:24Z

@raystorm Just wanted to update you here, we are aligned with adding the update to processFile and will be reviewing your PR. Thanks again for contributing!

calebpollman · 2024-05-08T17:18:30Z

@raystorm We recently made some updates to the StorageManager to support new functionality exposed on the uploadData API (consumed from aws-amplify/storage) used by the component, which has caused some merge conflicts with between main and your PR. Can help out with resolvng the merge conflicts, but wanted to give you the option to handle resolvng them i you prefer

* move ProcessFile Error/reject handling to `getInput()`

raystorm · 2024-05-10T01:15:03Z

@raystorm We recently made some updates to the StorageManager to support new functionality exposed on the uploadData API (consumed from aws-amplify/storage) used by the component, which has caused some merge conflicts with between main and your PR. Can help out with resolvng the merge conflicts, but wanted to give you the option to handle resolvng them i you prefer

Merge Conflicts are resolved. You may want to run the test example from my branch locally, to make sure everything still works properly. The Error Handling code is the same, but moved to getInput() I lost my local setup for running the examples, when I cleaned up some accidentally pushed env changes on my side. So I was unable to re-run the examples locally. as a secondary validation of my changes this time.

raystorm · 2024-06-01T18:44:23Z

Not to be impatient, but is there any expected timeline for review/merge/release?

calebpollman · 2024-06-04T01:35:48Z

Hey @raystorm, valid question. Have started reviewing, will finish the review in the next couple days. Thanks for your patience!

calebpollman

@raystorm Great start! The exception handling logic updates require changes to prevent calls to uploadFile on processFile error. Did not get in to reviewing the unit tests as they will need to be updated after the required changes are made

CONTRIBUTING.md

calebpollman · 2024-06-07T15:15:18Z

packages/react/CONTRIBUTING.md

-1. Run `yarn setup`
+1. [`nvm install`](https://github.com/nvm-sh/nvm)
+1. [`nvm use`](https://github.com/nvm-sh/nvm)
+1. Run `yarn setup` - _NOTE:_ if Cypress Fails to download during this step, delete `yarn.lock` and try again.


As noted previously, regenerating the yarn.lock can lead to unintended side effects. Please remove this change

packages/react/CONTRIBUTING.md

calebpollman · 2024-06-07T15:32:52Z

packages/react-storage/src/components/StorageManager/utils/getInput.ts

+    } = await resolveFile({ file, key, processFile }).catch(
+      (result: ProcessFileParams) => {
+        const { key, error } = result;
+        if (isFunction(onUploadError)) {


Because the caught exception here is from resolveFile and not an upload error, this usage of onUploadError is incorrect and needs to be removed

This has been commented out as incorrect, with a note, for a handler that might need to be added. Let me know if I should remove the code and the comment.

packages/react-storage/src/components/StorageManager/utils/getInput.ts

examples/next/pages/ui/components/storage/storage-manager/process-file-reject/index.page.tsx

calebpollman · 2024-06-07T16:11:02Z

packages/react-storage/src/components/StorageManager/types.ts

@@ -39,6 +39,7 @@ export type DefaultFile = Pick<StorageFile, 'key'>;
 export interface ProcessFileParams extends Record<string, any> {
  file: File;
  key: string;
+  error?: string;


This needs to be removed, processFile should throw the underlying exception and be captured by the caller

raystorm · 2024-06-19T15:43:08Z

...-storage/src/components/StorageManager/hooks/useUploadFiles/__tests__/useUploadFiles.spec.ts

+  it('should remove upload after processFile promise rejects', async () => {
+    const processFile: StorageManagerProps['processFile'] = ({ file }) =>
+      Promise.reject({ file, key: 'test.png', error: 'forced test error' });
+
+    const { waitForNextUpdate } = renderHook(() =>
+      useUploadFiles({
+        ...props,
+        isResumable: true,
+        processFile,
+        files: [mockQueuedFile],
+      })
+    );
+
+    waitForNextUpdate();
+
+    await waitFor(() => {
+      expect(mockOnUploadError).toHaveBeenCalledWith('forced test error', {
+        key: 'test.png',
+      });
+    });
+
+    await waitFor(() => {
+      expect(mockRemoveUpload).toHaveBeenCalled();
+    });
+  });


@calebpollman, With your comments on my example is this Unit Test incorrect?
Should I only be testing a thrown Exception? or is that a second test case?

+ add new test case for `processFile` to verify throw errors are handled properly - remove `onUploadError` after processFile errors/rejects. - remove `yarn.lock` notes per PR Comments. + Add Prequesities to `react/CONTIRUBUTING.MD`

Update packages/react/CONTRIBUTING.md per PR Change Request Co-authored-by: Caleb Pollman <[email protected]>

- commented out error as an optional property * rename resolved -> rejected for clarity in `resolveFile.catch()` in `getInput()` * switched example from `Promise.reject()` to `throw new Error()`

raystorm · 2024-07-07T17:48:47Z

@calebpollman I've made a lot of the changes you requested as I understood them.
It appears we have different visions for how this is to behave. I was just running the example locally, and for me, when I run examples/next/pages/ui/components/storage/storage-manager/process-file-reject it silently fails. So how should the end-user be notified that the uploaded file was rejected?

raystorm · 2024-07-07T17:57:02Z

@calebpollman Looking at the latest merge conflicts with the new onProcessFileSuccess() method, are you now handling this ask internally in a different way? Should I expect this PR to be closed and not merged in favor of your internal solution?

raystorm · 2024-08-05T23:59:32Z

@calebpollman I've made a lot of the changes you requested as I understood them. It appears we have different visions for how this is to behave. I was just running the example locally, and for me, when I run examples/next/pages/ui/components/storage/storage-manager/process-file-reject it silently fails. So how should the end-user be notified that the uploaded file was rejected?

@reesscot Are you on the team for this as well?

Having not seen a response, I am unsure how to proceed?
Thinking about it, I think a new optional function is needed, for the end user. onProcessFileError() or similar. Will there be any problems if I add a new function to the API? Or are you solving this internally?

calebpollman · 2024-08-06T00:48:06Z

Hey @raystorm, there's some problem areas in the upload input resolution we need to sort out in order to correctly address this. Would still like to get this in, but thinking that it might be more straightforward if we integrate your changes in with the input resolution refactoring, does that work for you?

raystorm · 2024-08-06T01:54:24Z

I just want this resolved.
This is a blocking issue in my application right now.
I didn't expect this to take this long.
I'm just confused on what's going on, and the nature if the most recent updates with the onSuccess method, and what that means for my change.

Is there something I need to do to help move things forward?

calebpollman · 2024-08-07T22:47:25Z

@raystorm Apologies here - there has been some churn on our end and we will work with you to get this resolved.

Regarding onProcessFileSuccess, it was added to address an issue with the value provided to onUploadSuccess not being the value that was provided to the upload call if updated through processFile, it was not intended nor it should impact the behavior you are implementing here.

+ add note about deleteing yarn.lock, so Cypress will download/install correctly + add `nvm install` & `nvm use` to react file.

add changeset

+ add new test case for `processFile` to verify throw errors are handled properly - remove `onUploadError` after processFile errors/rejects. - remove `yarn.lock` notes per PR Comments. + Add Prequesities to `react/CONTIRUBUTING.MD`

- commented out error as an optional property * rename resolved -> rejected for clarity in `resolveFile.catch()` in `getInput()` * switched example from `Promise.reject()` to `throw new Error()`

+ Add new `OnProcessFileError` function & `ProcessFileErrorParams` object * move error processing to inside resolveFile directly where processFile used. * update example with the new function.

…l-upload-from-processFile

…-processFile' into react-storage/cancel-upload-from-processFile # Conflicts: # examples/next/pages/ui/components/storage/storage-manager/process-file-reject/index.page.tsx # packages/react-storage/src/components/StorageManager/hooks/useUploadFiles/__tests__/useUploadFiles.spec.ts # packages/react-storage/src/components/StorageManager/hooks/useUploadFiles/useUploadFiles.ts # packages/react-storage/src/components/StorageManager/types.ts # packages/react-storage/src/components/StorageManager/utils/__tests__/getInput.spec.ts # packages/react-storage/src/components/StorageManager/utils/getInput.ts # turbo.json

raystorm · 2024-08-25T03:53:45Z

@raystorm Apologies here - there has been some churn on our end and we will work with you to get this resolved.

Regarding onProcessFileSuccess, it was added to address an issue with the value provided to onUploadSuccess not being the value that was provided to the upload call if updated through processFile, it was not intended nor it should impact the behavior you are implementing here.

I've done my best to FINALLY, get the merge conflicts resolved AGAIN.
I've also added a new function onProcessFileError and a new type, onProcessFileErrorParams. I've moved the error handling into resolveFile where it should have been in the first place so it directly responds to process file throwing an exception or rejecting.
Hopefully the new code meets with approval and we can finally get this PR done.

reesscot

There's a lot of files being changed in this PR that aren't related to your change, so we're not going to be able to merge this as is. Can you please squash your changes and rebase them against the latest version of main? Looks like something went wrong with resolving merge conflicts.

reesscot · 2024-08-28T22:13:19Z

.changeset/short-doors-flash.md

+"@aws-amplify/ui-react-storage": patch
+---
+
+fix(storage-manager): update file processedKey on process file complete


I don't think this file is needed

reesscot · 2024-08-28T22:13:47Z

...orm]/connected-components/liveness/customization/customization.system-clock-offset.react.mdx

@@ -14,30 +14,25 @@ function MyComponent() {
  const [createLivenessApiData, setCreateLivenessApiData] = React.useState(null);

  /* 
-   * 1. Check whether the difference between server time and device time is
-   *  greater than or equal to 5 minutes, and if so, return the offset in milliseconds.
-   *  This logic should be adjusted based on the server response and use case


Are these changes related to your PR? Same comment on all the other liveness files.

reesscot · 2024-08-28T22:14:24Z

packages/react-core/src/hooks/__tests__/useDataState.spec.ts

@@ -26,7 +26,6 @@ describe('useDataState', () => {
      expect(action).not.toHaveBeenCalled();

      expect(initState.data).toBe(initData);
-      expect(initState.hasError).toBe(false);


This file shouldn't be getting changed

raystorm · 2024-08-30T02:35:17Z

There's a lot of files being changed in this PR that aren't related to your change, so we're not going to be able to merge this as is. Can you please squash your changes and rebase them against the latest version of main? Looks like something went wrong with resolving merge conflicts.

@reesscot These changes are a result of a merge issue when pulling in the latest version from main to fix some merge issues, blocking the PR from being mergable. I'm not good enough with Git to quickly squash and rebase. So instead I've created a new branch and PR #5717
I am closing this PR for that one.

raystorm added 4 commits April 24, 2024 08:41

Get yarn setup to run locally

5fce159

Update CONTRIBUTING.md files

7957bf1

+ add note about deleteing yarn.lock, so Cypress will download/install correctly + add `nvm install` & `nvm use` to react file.

Ability to cancel uploads in processFile - implements aws-amplify#5099

9af00ac

Add Example to locally test rejecting an upload in ProcessFile.

e2cac8e

raystorm requested a review from a team as a code owner April 24, 2024 21:22

raystorm added 2 commits April 24, 2024 17:35

Remove Environment changes

1639c38

add changeset

Merge remote-tracking branch 'upstream/main' into react-storage/cance…

29afbdb

…l-upload-from-processFile # Conflicts: # yarn.lock

Merge in Upstream Changes to fix Merge Conflicts.

db02fa7

* move ProcessFile Error/reject handling to `getInput()`

Merge branch 'main' into react-storage/cancel-upload-from-processFile

92400ec

Merge branch 'main' into react-storage/cancel-upload-from-processFile

558098e

calebpollman reviewed Jun 7, 2024

View reviewed changes

raystorm commented Jun 19, 2024

View reviewed changes

raystorm and others added 5 commits June 19, 2024 12:17

Initial PR Updates

7f96d37

+ add new test case for `processFile` to verify throw errors are handled properly - remove `onUploadError` after processFile errors/rejects. - remove `yarn.lock` notes per PR Comments. + Add Prequesities to `react/CONTIRUBUTING.MD`

Update packages/react/CONTRIBUTING.md

1b54c48

Update packages/react/CONTRIBUTING.md per PR Change Request Co-authored-by: Caleb Pollman <[email protected]>

Update packages/react/CONTRIBUTING.md

a54b074

Update packages/react/CONTRIBUTING.md per PR Change Request Co-authored-by: Caleb Pollman <[email protected]>

Update packages/react/CONTRIBUTING.md

b75e330

Update packages/react/CONTRIBUTING.md per PR Change Request Co-authored-by: Caleb Pollman <[email protected]>

More PR Updates

742e226

- commented out error as an optional property * rename resolved -> rejected for clarity in `resolveFile.catch()` in `getInput()` * switched example from `Promise.reject()` to `throw new Error()`

raystorm added 9 commits August 14, 2024 21:42

Update CONTRIBUTING.md files

f5f5600

+ add note about deleteing yarn.lock, so Cypress will download/install correctly + add `nvm install` & `nvm use` to react file.

Ability to cancel uploads in processFile - implements aws-amplify#5099

eb8e738

Add Example to locally test rejecting an upload in ProcessFile.

977a483

Remove Environment changes

49101e9

add changeset

Initial PR Updates

cafd27d

+ add new test case for `processFile` to verify throw errors are handled properly - remove `onUploadError` after processFile errors/rejects. - remove `yarn.lock` notes per PR Comments. + Add Prequesities to `react/CONTIRUBUTING.MD`

More PR Updates

dc4bb15

- commented out error as an optional property * rename resolved -> rejected for clarity in `resolveFile.catch()` in `getInput()` * switched example from `Promise.reject()` to `throw new Error()`

Fix merge conflicts and Notify Users on processFile errors

1ef9f41

+ Add new `OnProcessFileError` function & `ProcessFileErrorParams` object * move error processing to inside resolveFile directly where processFile used. * update example with the new function.

Merge remote-tracking branch 'upstream/main' into react-storage/cance…

46fec56

…l-upload-from-processFile

reesscot requested changes Aug 28, 2024

View reviewed changes

raystorm mentioned this pull request Aug 30, 2024

Add Ability to Cancel File upload from `processF… #5717

Open

5 tasks

raystorm closed this Aug 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

React storage/cancel upload from process file #5179

React storage/cancel upload from process file #5179

raystorm commented Apr 24, 2024

changeset-bot bot commented Apr 24, 2024 •

edited

Loading

calebpollman commented Apr 24, 2024 •

edited

Loading

calebpollman commented Apr 26, 2024

calebpollman commented May 8, 2024

raystorm commented May 10, 2024

raystorm commented Jun 1, 2024

calebpollman commented Jun 4, 2024

calebpollman left a comment

calebpollman Jun 7, 2024

calebpollman Jun 7, 2024

raystorm Jun 19, 2024

calebpollman Jun 7, 2024

raystorm Jun 19, 2024

raystorm commented Jul 7, 2024 •

edited

Loading

raystorm commented Jul 7, 2024

raystorm commented Aug 5, 2024

calebpollman commented Aug 6, 2024 •

edited

Loading

raystorm commented Aug 6, 2024

calebpollman commented Aug 7, 2024

raystorm commented Aug 25, 2024

reesscot left a comment

reesscot Aug 28, 2024

reesscot Aug 28, 2024

reesscot Aug 28, 2024

raystorm commented Aug 30, 2024

React storage/cancel upload from process file #5179

React storage/cancel upload from process file #5179

Conversation

raystorm commented Apr 24, 2024

Description of changes

Issue #, if available

Description of how you validated changes

Checklist

changeset-bot bot commented Apr 24, 2024 • edited Loading

🦋 Changeset detected

calebpollman commented Apr 24, 2024 • edited Loading

calebpollman commented Apr 26, 2024

calebpollman commented May 8, 2024

raystorm commented May 10, 2024

raystorm commented Jun 1, 2024

calebpollman commented Jun 4, 2024

calebpollman left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raystorm commented Jul 7, 2024 • edited Loading

raystorm commented Jul 7, 2024

raystorm commented Aug 5, 2024

calebpollman commented Aug 6, 2024 • edited Loading

raystorm commented Aug 6, 2024

calebpollman commented Aug 7, 2024

raystorm commented Aug 25, 2024

reesscot left a 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

raystorm commented Aug 30, 2024

changeset-bot bot commented Apr 24, 2024 •

edited

Loading

calebpollman commented Apr 24, 2024 •

edited

Loading

raystorm commented Jul 7, 2024 •

edited

Loading

calebpollman commented Aug 6, 2024 •

edited

Loading