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.

Sign up for GitHub

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(Field.Upload): handling of multiple async file uploads #4360

Merged
merged 5 commits into from
Dec 6, 2024
+167 −8
Merged

fix(Field.Upload): handling of multiple async file uploads #4360

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2153fa9
fix(Field.Upload): improves multiple async uploads
langz Dec 5, 2024
eec9b68
Update packages/dnb-eufemia/src/extensions/forms/Field/Upload/__tests…
langz Dec 5, 2024
9d76bb7
Update packages/dnb-eufemia/src/extensions/forms/Field/Upload/Upload.tsx
langz Dec 5, 2024
1c7cc70
updates test
langz Dec 6, 2024
5fd43d9
Update packages/dnb-eufemia/src/extensions/forms/Field/Upload/__tests…
langz Dec 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 25 additions & 8 deletions packages/dnb-eufemia/src/extensions/forms/Field/Upload/Upload.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, { useCallback, useEffect, useMemo } from 'react'
import React, { useCallback, useEffect, useMemo, useRef } from 'react'
import classnames from 'classnames'
import FieldBlock, {
Props as FieldBlockProps,
Expand Down Expand Up @@ -117,6 +117,12 @@ function UploadComponent(props: Props) {

const { files: fileContext, setFiles } = useUpload(id)

const filesRef = useRef<Array<UploadFile>>()

useEffect(() => {
filesRef.current = fileContext
}, [fileContext])

useEffect(() => {
tujoworker marked this conversation as resolved.
Show resolved Hide resolved
// Files stored in session storage will not have a property (due to serialization).
const hasInvalidFiles = value?.some(({ file }) => !file?.name)
Expand All @@ -126,10 +132,10 @@ function UploadComponent(props: Props) {
}, [setFiles, value])

const handleChangeAsync = useCallback(
async (files: UploadValue) => {
async (existingFiles: UploadValue) => {
// Filter out existing files
const existingFileIds = fileContext?.map((file) => file.id) || []
const newFiles = files.filter(
const newFiles = existingFiles.filter(
(file) => !existingFileIds.includes(file.id)
)

Expand All @@ -140,15 +146,26 @@ function UploadComponent(props: Props) {
...updateFileLoadingState(newFiles, { isLoading: true }),
])

const uploadedFiles = updateFileLoadingState(
await fileHandler(newFiles),
{ isLoading: false }
const incomingFiles = await fileHandler(newFiles)

const uploadedFiles = updateFileLoadingState(incomingFiles, {
isLoading: false,
})

const indexOfFirstNewFile = filesRef.current.findIndex(
({ id }) => id === newFiles[0].id
Copy link
Member

Choose a reason for hiding this comment

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

What is the reason we do it that way? What if there is one file added already. And later the user adds two. Will the second one ever be newFiles[0]?

Copy link
Contributor Author

@langz langz Dec 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We can probably do it in a different way, somehow this is the only one that made sense to me at the given time.
What I do is that I find the first "new file", and use that to find the index of where the "new files" is located in the files (the new files is always located/positioned after each other). I then remove/replace these new files with the async received files.

So what I want to do is to update the latest files by replacing the "new files" (the files newly added by the user) with the async received files, and keeping the same order.

Copy link
Member

Choose a reason for hiding this comment

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

Right. Can you try if this works as well? I think it should do the same as the slicing:

const updatedFiles = filesRef.current.map((file) => {
  const matchingNewFile = uploadedFiles.find(({ id }) => id === file.id)
  return matchingNewFile || file
})

Copy link
Contributor Author

@langz langz Dec 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the great suggestion.
The problem is that the id of the uploaded/async file can be something other than the local id, as it can be server generated, as in the example setting server_generated_id.
But it probably exists a more delicate way of solving this, than what I've done so far 🧠
Maybe using a different property? But then again, the response from the server could potentially change whatever property it wants.

Copy link
Member

Choose a reason for hiding this comment

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

I see, it does not the same 😀 – but I think now we are on something:

// Update files, replacing newFiles with uploadedFiles, preserving order
const updatedFiles = [
  ...filesRef.current.map(
    (file) =>
      uploadedFiles.find((uploaded) => uploaded.id === file.id) ||
      file
  ),
  ...uploadedFiles.filter(
    (uploaded) =>
      !filesRef.current.some((file) => file.id === uploaded.id)
  ),
]

But it's not less code, so I'm not sure it's better 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll merge now, then try this improvement later.
I see that this again also depends on the id, which I again think can be different in what's coming from the "server".

)

const updatedFiles = [
...filesRef.current.slice(0, indexOfFirstNewFile),
...uploadedFiles,
...filesRef.current.slice(indexOfFirstNewFile + newFiles.length),
]

// Set error, if any
handleChange([...fileContext, ...uploadedFiles])
handleChange(updatedFiles)
} else {
handleChange(files)
handleChange(existingFiles)
}
},
[fileContext, setFiles, fileHandler, handleChange]
Expand Down
85 changes: 85 additions & 0 deletions packages/dnb-eufemia/src/extensions/forms/Field/Upload/__tests__/Upload.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1154,6 +1154,91 @@
})
})

it('should add new files from fileHandler with async function with multiple actions', async () => {
langz marked this conversation as resolved.
Show resolved Hide resolved
const newFile = (fileId) => {
return createMockFile(`${fileId}.png`, 100, 'image/png')
}

const files = [
newFile(0),
newFile(1),
newFile(2),
newFile(3),
newFile(4),
newFile(5),
]

const asyncValidatorResolvingWithSuccess = (id) =>
new Promise<UploadValue>((resolve) =>
setTimeout(
() =>
resolve([
{
file: files[id],
id: 'server_generated_id_' + id,
exists: false,
},
]),
1
)
)

const asyncValidatorNeverResolving = () =>
new Promise<UploadValue>(() => {})

Check failure on line 1187 in packages/dnb-eufemia/src/extensions/forms/Field/Upload/__tests__/Upload.test.tsx

View workflow job for this annotation

GitHub Actions / Run tests and checks 

Unexpected empty arrow function
langz marked this conversation as resolved.
Show resolved Hide resolved

const asyncFileHandlerFnSuccess = jest
.fn(asyncValidatorResolvingWithSuccess)
.mockReturnValueOnce(asyncValidatorResolvingWithSuccess(0))
.mockReturnValueOnce(asyncValidatorNeverResolving())
.mockReturnValueOnce(asyncValidatorResolvingWithSuccess(2))
.mockReturnValueOnce(asyncValidatorNeverResolving())

render(<Field.Upload fileHandler={asyncFileHandlerFnSuccess} />)

const element = getRootElement()

await waitFor(() => {
fireEvent.drop(element, {
dataTransfer: {
files: [files[0]],
},
})

fireEvent.drop(element, {
dataTransfer: {
files: [files[1], files[3], files[4]],
},
})

fireEvent.drop(element, {
dataTransfer: {
files: [files[2]],
},
})

fireEvent.drop(element, {
dataTransfer: {
files: [files[5]],
},
})

expect(
document.querySelectorAll('.dnb-upload__file-cell').length
).toBe(6)

expect(screen.queryByText('0.png')).toBeInTheDocument()
expect(screen.queryByText('1.png')).not.toBeInTheDocument()
expect(screen.queryByText('2.png')).toBeInTheDocument()
expect(screen.queryByText('3.png')).not.toBeInTheDocument()
expect(screen.queryByText('4.png')).not.toBeInTheDocument()
expect(screen.queryByText('5.png')).not.toBeInTheDocument()

expect(
document.querySelectorAll('.dnb-progress-indicator').length
).toBe(4)
})
})

it('should not add existing file using fileHandler with async function', async () => {
const file = createMockFile('fileName.png', 100, 'image/png')

Expand Down
57 changes: 57 additions & 0 deletions packages/dnb-eufemia/src/extensions/forms/Field/Upload/stories/Upload.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,3 +96,60 @@ export const WithAsyncFileHandler = () => {
</Form.Handler>
)
}

export const AsyncEverything = () => {
const acceptedFileTypes = ['jpg', 'pdf', 'png']

async function mockAsyncFileRemoval({ fileItem }) {
const request = createRequest()
console.log('making API request to remove: ' + fileItem.file.name)
await request(3000) // Simulate a request
}

async function mockAsyncFileUpload(
newFiles: UploadValue
): Promise<UploadValue> {
const updatedFiles: UploadValue = []

for (const [, file] of Object.entries(newFiles)) {
const formData = new FormData()
formData.append('file', file.file, file.file.name)

const request = createRequest()
await request(3000) // Simulate a request

const mockResponse = {
ok: false, // Fails virus check
json: async () => ({
server_generated_id:
'server_generated_id' +
'_' +
file.file.name +
'_' +
crypto.randomUUID(),
}),
}

const data = await mockResponse.json()
updatedFiles.push({
...file,
id: data.server_generated_id,
})
}

return updatedFiles
}

return (
<Form.Handler onSubmit={async (form) => console.log(form)}>
<Flex.Stack>
<Field.Upload
onFileDelete={mockAsyncFileRemoval}
fileHandler={mockAsyncFileUpload}
id="upload-example-async"
acceptedFileTypes={acceptedFileTypes}
/>
</Flex.Stack>
</Form.Handler>
)
}
Loading