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

Why eat the possible errors in the upload ? #66

Open
vegandiet705 opened this issue Jun 23, 2024 · 2 comments
Open

Why eat the possible errors in the upload ? #66

vegandiet705 opened this issue Jun 23, 2024 · 2 comments

Comments

@vegandiet705
Copy link

Hello,

In the file src/react/useNextUpload.tsx, in the upload function returned by the hook useNextUpload, why does it eat the possible errors ? Is it for security reasons, to not let confidential information leaking to the client ? Then, why not throw a generic error instead ?

In my use case, I'm finding myself with some failed upload requests (due to external reasons) and since the errors are eaten, the app continues the flow and I end up with corrupted orders in my database, which can result in a bad deliver for my clients.

If it's indeed a bug, I can propose a PR. If it's not, let me know your reasons, please.

Thanks

@TimMikeladze
Copy link
Owner

This is indeed an oversight. PRs welcome.

@vegandiet705
Copy link
Author

vegandiet705 commented Jun 24, 2024

@TimMikeladze Actually for my use case, I think it's better to use a server action like the following:

'use server'

import { nup } from '@/app/upload/nup';

async function idExistInBucket(id: string): Promise<boolean> {
    try {
        await nup.init();
        await nup.getAsset({ id });
    }
    catch (error) {
        return false;
    }

    return true;
}

export async function idsExistInBucket(ids: string[]): Promise<Map<string, boolean>> {
    const resultMap = new Map<string, boolean>();
    const promises = ids.map(async id => {
        const exists = await idExistInBucket(id);
        resultMap.set(id, exists);
    });

    await Promise.all(promises);

    return resultMap;
}

With this server action, I can do the existence verification before uploading files in the client, avoiding upload requests for already existent files:

    const uploadFiles = async () => {
        const orderAndFilesArray = await cacheOperator.getOrderAndFilesArray(orderIds);

        await Promise.all(
            orderAndFilesArray.map(async (orderAndFiles) => {
                const files = orderAndFiles.files.map((file) => {
                    return { id: `${file.imageId}`, file: blobToFile(arrayBufferToBlob(file.file, file.type), `${file.imageId}`) };
                });
                const filesIds = files.map(file => file.id);
                const resultMap = await idsExistInBucket(filesIds);
                const uploadPromises = [];

                for (const file of files) {
                    if (!resultMap.get(file.id)) {
                        uploadPromises.push(nup.upload(file));
                    }
                }

                await Promise.all(uploadPromises);
            })
        );
    };

This not only ensures error prevention but also idempotence. So, I'm retreating on my PR suggestion for now. But how do you think it should be done ? My idea was just adding a throw("Generic error") inside the upload's catch block, in the file src/react/useNextUpload.tsx.

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

No branches or pull requests

2 participants