aws-amplify / amplify-ui

Amplify UI is a collection of accessible, themeable, performant React (and more!) components that can connect directly to the cloud.
https://ui.docs.amplify.aws
Apache License 2.0
912 stars 294 forks source link

onUploadSuccess not triggered #5800

Open nam-truong-le opened 2 months ago

nam-truong-le commented 2 months ago

Before creating a new issue, please confirm:

On which framework/platform are you having an issue?

React

Which UI component?

Storage

How is your app built?

amplify gen2 nextjs template

What browsers are you seeing the problem on?

Chrome

Which region are you seeing the problem in?

Frankfurt

Please describe your bug.

In my app onUploadStart is fired but onUploadSuccess not. Upload succeeds.

What's the expected behaviour?

onUploadSuccess should be fired.

Help us reproduce the bug!

Please check the code snippet.

Code Snippet

  const onUploadStart = useCallback(() => {
    dispatch(setStatus("loading"));
    console.log("uploading");
  }, [dispatch]);

  const onUploadSuccess = useCallback(() => {
    debugger;
    dispatch(setStatus("idle"));
    console.log("uploaded");
  }, [dispatch]);

<FileUploader
            isResumable
            onUploadStart={onUploadStart}
            onUploadSuccess={onUploadSuccess}
            onUploadError={onUploadSuccess}
            maxFileCount={1}
            path={`applicant-photos/${today}/`}
            acceptedFileTypes={["image/*"]}
          />

Console log output

No response

Additional information and screenshots

No response

jordanvn commented 2 months ago

Hi @nam-truong-le, thanks for submitting this issue. Unfortunately, we have not been able to reproduce this behavior with the FileUploader in a Next.js app. We will need more information to figure out why you're experiencing this.

To confirm, is this the template you're using for your app? https://github.com/aws-samples/amplify-next-template

Additionally, would you be able to share your package.json contents?

nam-truong-le commented 2 months ago

Yes, https://github.com/aws-samples/amplify-next-template is the template. My package.json looks like this

{
  "name": "aws-amplify-gen2",
  "version": "0.3.0",
  "private": true,
  "scripts": {
    "dev": "NODE_ENV=development next dev -p 3999",
    "build": "next build",
    "start": "next start",
    "lint": "next lint",
    "format": "prettier --write .",
    "format:check": "prettier --check .",
    "sandbox": "env-cmd -f .env.development ampx sandbox --stream-function-logs --profile vs2 --identifier truong",
    "sandbox:delete": "ampx sandbox delete --profile vs2 --identifier truong"
  },
  "dependencies": {
    "@aws-amplify/adapter-nextjs": "1.2.17",
    "@aws-amplify/ui-react": "6.1.13",
    "@aws-amplify/ui-react-storage": "3.3.2",
    "@emotion/react": "11.13.3",
    "@emotion/styled": "11.13.0",
    "@fontsource/roboto": "5.1.0",
    "@mui/icons-material": "6.1.0",
    "@mui/material": "6.1.0",
    "@mui/material-nextjs": "5.16.6",
    "@mui/types": "7.2.15",
    "@paypal/paypal-js": "8.1.0",
    "@paypal/react-paypal-js": "8.6.0",
    "@reduxjs/toolkit": "2.2.7",
    "@vietnamvisa/vs2-react": "1.6.11",
    "aws-amplify": "6.4.0",
    "crypto-random-string": "5.0.0",
    "dayjs": "1.11.13",
    "lodash.concat": "4.5.0",
    "lodash.difference": "4.5.0",
    "lodash.filter": "4.6.0",
    "lodash.groupby": "4.6.0",
    "lodash.sortby": "4.7.0",
    "lodash.sum": "4.0.2",
    "lodash.sumby": "4.6.0",
    "lodash.truncate": "4.4.2",
    "next": "14.2.11",
    "react": "^18",
    "react-dom": "^18",
    "react-redux": "9.1.2",
    "schema-dts": "1.1.2"
  },
  "devDependencies": {
    "@aws-amplify/backend": "1.2.1",
    "@aws-amplify/backend-cli": "1.2.6",
    "@types/lodash.concat": "4.5.9",
    "@types/lodash.difference": "4.5.9",
    "@types/lodash.filter": "4.6.9",
    "@types/lodash.groupby": "4.6.9",
    "@types/lodash.sortby": "4.7.9",
    "@types/lodash.sum": "4.0.9",
    "@types/lodash.sumby": "4.6.9",
    "@types/lodash.truncate": "4.4.9",
    "@types/node": "20.16.5",
    "@types/react": "18.3.5",
    "@types/react-dom": "18.3.0",
    "ampx": "0.2.1",
    "aws-cdk": "2.158.0",
    "env-cmd": "10.1.0",
    "eslint": "8.57.0",
    "eslint-config-next": "14.2.11",
    "eslint-config-prettier": "8.10.0",
    "prettier": "3.3.3",
    "typescript": "5.6.2"
  },
  "peerDependencies": {
    "react": "^17.0.0 || ^18.0.0",
    "react-dom": "^17.0.0 || ^18.0.0"
  },
  "packageManager": "pnpm@9.10.0+sha512.73a29afa36a0d092ece5271de5177ecbf8318d454ecd701343131b8ebc0c1a91c487da46ab77c8e596d6acf1461e3594ced4becedf8921b074fbd8653ed7051c"
}
nam-truong-le commented 2 months ago

If I enable useAccelerateEndpoint, then request fails because of CORS and onUploadError is fired.

nam-truong-le commented 2 months ago

After some packages are updated, the other issue reported here got resolved https://github.com/aws-amplify/amplify-ui/issues/5801

But this issue still persist. This is the updated package.json:

{
  "name": "aws-amplify-gen2",
  "version": "0.3.0",
  "private": true,
  "scripts": {
    "dev": "NODE_ENV=development next dev -p 3999",
    "build": "next build",
    "start": "next start",
    "lint": "next lint",
    "format": "prettier --write .",
    "format:check": "prettier --check .",
    "sandbox": "env-cmd -f .env.development ampx sandbox --stream-function-logs --profile vs2 --identifier truong",
    "sandbox:delete": "ampx sandbox delete --profile vs2 --identifier truong"
  },
  "dependencies": {
    "@aws-amplify/adapter-nextjs": "1.2.18",
    "@aws-amplify/ui-react": "6.3.1",
    "@aws-amplify/ui-react-storage": "3.3.2",
    "@emotion/react": "11.13.3",
    "@emotion/styled": "11.13.0",
    "@fontsource/roboto": "5.1.0",
    "@mui/icons-material": "6.1.0",
    "@mui/material": "6.1.0",
    "@mui/material-nextjs": "6.1.0",
    "@mui/types": "7.2.16",
    "@paypal/paypal-js": "8.1.2",
    "@paypal/react-paypal-js": "8.7.0",
    "@reduxjs/toolkit": "2.2.7",
    "@vietnamvisa/vs2-react": "1.6.11",
    "aws-amplify": "6.6.1",
    "crypto-random-string": "5.0.0",
    "dayjs": "1.11.13",
    "lodash.concat": "4.5.0",
    "lodash.difference": "4.5.0",
    "lodash.filter": "4.6.0",
    "lodash.groupby": "4.6.0",
    "lodash.sortby": "4.7.0",
    "lodash.sum": "4.0.2",
    "lodash.sumby": "4.6.0",
    "lodash.truncate": "4.4.2",
    "next": "14.2.11",
    "react": "^18",
    "react-dom": "^18",
    "react-redux": "9.1.2",
    "schema-dts": "1.1.2"
  },
  "devDependencies": {
    "@aws-amplify/backend": "1.2.1",
    "@aws-amplify/backend-cli": "1.2.6",
    "@types/lodash.concat": "4.5.9",
    "@types/lodash.difference": "4.5.9",
    "@types/lodash.filter": "4.6.9",
    "@types/lodash.groupby": "4.6.9",
    "@types/lodash.sortby": "4.7.9",
    "@types/lodash.sum": "4.0.9",
    "@types/lodash.sumby": "4.6.9",
    "@types/lodash.truncate": "4.4.9",
    "@types/node": "20.16.5",
    "@types/react": "18.3.6",
    "@types/react-dom": "18.3.0",
    "ampx": "0.2.1",
    "aws-cdk": "2.158.0",
    "env-cmd": "10.1.0",
    "eslint": "8.57.1",
    "eslint-config-next": "14.2.11",
    "eslint-config-prettier": "8.10.0",
    "prettier": "3.3.3",
    "typescript": "5.6.2"
  },
  "peerDependencies": {
    "react": "^17.0.0 || ^18.0.0",
    "react-dom": "^17.0.0 || ^18.0.0"
  },
  "packageManager": "pnpm@9.10.0+sha512.73a29afa36a0d092ece5271de5177ecbf8318d454ecd701343131b8ebc0c1a91c487da46ab77c8e596d6acf1461e3594ced4becedf8921b074fbd8653ed7051c"
}
nam-truong-le commented 2 months ago

onStart works perfectly but onComplete not.

Bildschirmfoto 2024-09-17 um 10 20 09
nam-truong-le commented 2 months ago

The utils function uploadFile doesn't resolve so .then(...) isn't triggered:

Bildschirmfoto 2024-09-17 um 10 27 12
nam-truong-le commented 2 months ago

I went to this file and what I can observe is: this packages/core/src/clients/internal/composeServiceApi.ts is executed twice

Bildschirmfoto 2024-09-17 um 12 20 21
esauerbo commented 2 months ago

Hey @nam-truong-le thanks for all the detailed information! I tried reproducing this with a simplified version of your package.json (I only included necessary dependencies for FileUploader) and removed the dispatch logic, and onUploadSuccess was triggered.

It's possible this is related to the dispatch logic; can you try simplifying your onUploadSuccess to just include a console.log statement and let us know if that gets triggered?

nam-truong-le commented 2 months ago

I switched to this simple callbacks, without dispatch and without useCallback:

  const onUploadStart = () => {
    debugger;
    // dispatch(setStatus("loading"));
    console.log("uploading");
  };

  const onUploadSuccess = () => {
    debugger;
    // dispatch(setStatus("idle"));
    console.log("uploaded");
  };

  const onUploadFailure = () => {
    debugger;
    // dispatch(setStatus("failed"));
  };

          <FileUploader
            onUploadStart={() => onUploadStart()}
            onUploadSuccess={() => onUploadSuccess()}
            onUploadError={() => onUploadFailure()}
            maxFileCount={1}
            path={`applicant-photos/${today}/`}
            acceptedFileTypes={["image/*"]}
          />

But it's still not working.

nam-truong-le commented 2 months ago

Now I see that the component uploads the file twice to S3 and then somehow the two parallel processes breaking the whole thing?

nam-truong-le commented 2 months ago

The reason is this hook executes the upload twice:

Bildschirmfoto 2024-09-18 um 10 10 48
nam-truong-le commented 2 months ago

Looks like this is called too late:

Bildschirmfoto 2024-09-18 um 10 28 37

If this is called before the second time useUploadFiles is triggered, then file has status "uploading" and then the second upload isn't triggered.

nam-truong-le commented 2 months ago

Since onStart is executed asynchronously, it would be better to move the setUploadingFile(...) call to line 84 or after line 108, ensuring the file is immediately marked as "uploading." (I'm referring to this)

Logically, it also makes sense to flag the file as "uploading" as soon as uploadFile is invoked, rather than waiting for the onStart callback. There could be a lot happening between these two events, and if the useEffect function is triggered again during that time, the file might not be marked correctly.

Bildschirmfoto 2024-09-18 um 10 44 58
nam-truong-le commented 2 months ago

I made this change https://github.com/aws-amplify/amplify-ui/pull/5808 locally and tested and it works perfectly.

esauerbo commented 2 months ago

@nam-truong-le thanks for digging into this issue and creating a PR. We'll review as a team and follow up.

In the meantime, I'm still curious why the file is being uploaded twice to S3. Can you provide your backend configuration (your call to defineStorage)?

Additionally, can you try uploading a file without the FileUploader component and see if that results in the same issue? Something like this:

import React from 'react';
import { uploadData } from 'aws-amplify/storage';

function App() {
  const [file, setFile] = React.useState();

  const handleChange = (event: any) => {
    setFile(event.target.files[0]);
  };

  return (
    <div>
      <input type="file" onChange={handleChange} />
        <button
          onClick={async () => {
            try {
              const result = await uploadData({
                path: `applicant-photos/${today}/${file.name}`,
                data: file,
              }).result;
              console.log("Uploaded file: ", result);
            } catch (error) {
              console.error("Error uploading file: ", error);
            }
          }}
        >
        Upload
      </button>
    </div>
  );
}
nam-truong-le commented 2 months ago

This is my storage config:

import { defineStorage } from "@aws-amplify/backend";

export const storage = defineStorage({
  name: `shopAppStorage-${process.env.AWS_BRANCH}`,
  access: (allow) => ({
    "applicant-photos/*": [allow.guest.to(["write", "read"]), allow.authenticated.to(["write", "read"])],
  }),
});
nam-truong-le commented 2 months ago

I'm still curious why the file is being uploaded twice to S3

I think it happens because the file gets sent to S3 and marked as "uploading" a bit too late. Before that happens, React triggers the hook again (for some reason I'm not sure of), and at that point, the file still shows as "queued." and it gets sent again.

nam-truong-le commented 2 months ago

I have to change your code snippet a little bit but it works perfectly without the FileUploader:

  const [file, setFile] = useState<File | null>(null);

  const handleChange = (event: any) => {
    setFile(event.target.files[0]);
  };

<div>
            <input type="file" onChange={handleChange} />
            <button
              onClick={async () => {
                if (!file) {
                  return;
                }
                try {
                  const result = await uploadData({
                    path: `applicant-photos/${today}/${file.name}`,
                    data: file,
                  }).result;
                  console.log("Uploaded file: ", result);
                } catch (error) {
                  console.error("Error uploading file: ", error);
                }
              }}
            >
              Upload
            </button>
          </div>
Bildschirmfoto 2024-09-19 um 06 44 39
esauerbo commented 1 month ago

@nam-truong-le thanks for the information. Since you're consistently seeing this error, would you be able to provide a minimal reproduction?

This ideally would be a Github repository that contains the minimum amount of code/dependencies that still results in this behavior. Make sure to remove any sensitive information, and let us know if you need further guidance with that. This will help us narrow down the cause and validate potential solutions since we haven't been able to reproduce on our end.

nam-truong-le commented 1 month ago

Looks like uploadData isn't working now either. I dug into the xhr request, and it seems like addEventListener isn’t behaving properly. But setting onreadystatechange directly works just fine. Here is an example when I tested both approaches while debugging:

Bildschirmfoto 2024-09-22 um 08 42 30

Related code here: aws-amplify/amplify-js/packages/storage/src/providers/s3/utils/client/runtime/xhrTransferHandler.ts

nam-truong-le commented 1 month ago

Is it possible to get the pre-signed URL for a PUT request so that I can test the upload with e.g. fetch instead of xhr?

nam-truong-le commented 1 month ago

Is it possible to get the pre-signed URL for a PUT request so that I can test the upload with e.g. fetch instead of xhr?

My workaround:

It works well.

thaddmt commented 1 month ago

@nam-truong-le we recently released a patch which should fix the duplicate upload issue which may be related to this issue, could you try upgrading and let us know if you're still seeing the issue? - https://www.npmjs.com/package/@aws-amplify/ui-react-storage/v/3.3.5

esauerbo commented 1 month ago

@nam-truong-le are you still experiencing this issue?