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

<File.Upload /> infinite render loop when using transformIn property #4366

Closed
torryt opened this issue Dec 6, 2024 · 10 comments · Fixed by #4392
Closed

<File.Upload /> infinite render loop when using transformIn property #4366

torryt opened this issue Dec 6, 2024 · 10 comments · Fixed by #4392
Assignees
Labels

Comments

@torryt
Copy link

torryt commented Dec 6, 2024

🐛 Bug Report

Using the transformIn in the <File.Upload /> component cause an infinite render loop and the entire page will crash.

To Reproduce

Steps to reproduce the behavior:

https://codesandbox.io/p/sandbox/field-upload-infinite-render-loop-using-transformin-9gl7zy

Expected behavior

For context, we are trying to map the UploadValue object from the <File.Upload /> component to and from the following form state object:

interface DocumentMetadata {
  id: string
  fileName: string
}

Run npx envinfo

  System:
    OS: macOS 15.1.1
    CPU: (11) arm64 Apple M3 Pro
    Memory: 63.55 MB / 18.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 20.15.0 - ~/.nvm/versions/node/v20.15.0/bin/node
    Yarn: 1.22.22 - ~/.nvm/versions/node/v20.15.0/bin/yarn
    npm: 10.7.0 - ~/.nvm/versions/node/v20.15.0/bin/npm
    pnpm: 9.14.2 - ~/.nvm/versions/node/v20.15.0/bin/pnpm
  Managers:
    Homebrew: 4.4.8 - /opt/homebrew/bin/brew
    pip3: 24.2 - ~/miniconda3/bin/pip3
    RubyGems: 3.0.3.1 - /usr/bin/gem
  Utilities:
    Make: 3.81 - /usr/bin/make
    GCC: 16.0.0 - /usr/bin/gcc
    Git: 2.45.1 - /opt/homebrew/bin/git
    Clang: 16.0.0 - /usr/bin/clang
    Curl: 8.7.1 - /usr/bin/curl
    OpenSSL: 3.0.15 - /Users/torrytufteland/miniconda3/bin/openssl
  Servers:
    Apache: 2.4.62 - /usr/sbin/apachectl
  Virtualization:
    Docker: 27.3.1 - /usr/local/bin/docker
    Docker Compose: 2.30.3 - /usr/local/bin/docker-compose
  IDEs:
    IntelliJ: 2024.2.3
    VSCode: 1.95.3 - /usr/local/bin/code
    Vim: 9.0 - /usr/bin/vim
    Xcode: /undefined - /usr/bin/xcodebuild
  Languages:
    Bash: 3.2.57 - /bin/bash
    Java: 21.0.4 - /Users/torrytufteland/.sdkman/candidates/java/current/bin/javac
    Perl: 5.34.1 - /usr/bin/perl
    Python: 3.12.7 - /Users/torrytufteland/miniconda3/bin/python
    Python3: 3.12.7 - /Users/torrytufteland/miniconda3/bin/python3
    Ruby: 2.6.10 - /usr/bin/ruby
  Databases:
    SQLite: 3.45.3 - /Users/torrytufteland/miniconda3/bin/sqlite3
  Browsers:
    Chrome: 131.0.6778.109
    Edge: 131.0.2903.86
    Safari: 18.1.1

Eufemia Version

10.59.0

@tujoworker tujoworker self-assigned this Dec 9, 2024
@tujoworker tujoworker added bug Something isn't working and removed bug Something isn't working labels Dec 9, 2024
@tujoworker
Copy link
Member

tujoworker commented Dec 9, 2024

Hey Torry!

Here is a CSB with a working solution:

https://codesandbox.io/p/sandbox/eufemia-forms-value-upload-transform-4hfr88?file=%2Fsrc%2FApp.tsx%3A8%2C2&workspaceId=ws_LdVovV1oCo5S5p2KEnsf6m

But I'll take a look if we actually can fix the issue when returning a new instance.

@tujoworker
Copy link
Member

tujoworker commented Dec 9, 2024

I wasn’t able to find a solution, and here’s why:

  1. The field will receive the value either from the value/defaultValue prop or from the data context. It will run transformIn. It will then also try to set its value to the data context, if it not exists. But because it's an array, we can't compare it properly.
  2. The data context receives the new value.
  3. To sync this "path" with other fields or values that depend on it, a rerender is required. Rerenders only happen when the value has changed.
  4. Because a new array instance is considered a new value, this triggers a rerender.
  5. During the data context rerender (Form.Handler), the field checks if the value from the data context has changed. However, since transformIn needs to run again, it produces a new instance, making comparison "again" impossible. The field then tries to set the new value in the data context again, and the cycle repeats.

How can we deal with that problem?

My suggestion is to create a dedicated section in the docs for when dealing with objects/arrays in transformer functions.

Any other suggestions?

@torryt
Copy link
Author

torryt commented Dec 10, 2024

Sounds like a good solution @tujoworker 👍 Mutating the input instead of creating a new array works for us, and explaining it in the docs would help out future users of the transformer functions.

@tujoworker
Copy link
Member

Thanks for the feedback. I had another look and found a way to support it. I will create a PR soon.

I also think that your transformIn/transformOut needs to be the opposite. But I will add a full example in the docs for that use case. I think its good to have documented and tested.

Just to confirm your use case:

You want to store a different value in the data context than what is used in by the upload field itself. Is that correct?

@torryt
Copy link
Author

torryt commented Dec 11, 2024

Yes, that's correct! We want to store an object { id, fileName } in the data context. We don't need to store the file itself in state, since we have a custom onFileClick handler that use the id to download the file.

@tujoworker
Copy link
Member

since we have a custom onFileClick handler that use the id to download the file

If that's the only reason, I would recommend to not transform it. Because it (the File) should not be in the way when it is in the data context. Or is it?

In the provided example, I actually provide a "file cache", just to restore the file again: https://eufemia-git-fix-forms-field-transform-in-out-instance-eufemia.vercel.app/uilib/extensions/forms/feature-fields/more-fields/Upload/#transformin-and-transformout

Let me know what do you think about it. Also, this PR enhances the docs about what "in" and "out" means when using these transformers.

@tujoworker
Copy link
Member

I have updated the CSB with the new version.

@torryt
Copy link
Author

torryt commented Dec 13, 2024

If that's the only reason, I would recommend to not transform it. Because it (the File) should not be in the way when it is in the data context. Or is it?

Well, we submit the entire data context to the server, and the File object is potentially large. This will increase the request size, and makes the request object less readable.

In the provided example, I actually provide a "file cache", just to restore the file again: https://eufemia-git-fix-forms-field-transform-in-out-instance-eufemia.vercel.app/uilib/extensions/forms/feature-fields/more-fields/Upload/#transformin-and-transformout

Let me know what do you think about it. Also, this PR enhances the docs about what "in" and "out" means when using these transformers.
I like these docs 👍 It is good that you named the variables "external" and "internal" to clarify the flow and source of data.

Not sure about the purpose of the file cache though 🤔 How would this work if the user refresh the page, and the data is loaded from the DocumentMetadata[] object? There's no files present on the client side in that case.

@tujoworker
Copy link
Member

Well, we submit the entire data context to the server, and the File object is potentially large. This will increase the request size, and makes the request object less readable.

Yes, I totally understand this. And you can use that technique. You may else have a look on other techniques like filterData.

Not sure about the purpose of the file cache though 🤔 How would this work if the user refresh the page, and the data is loaded from the DocumentMetadata[] object? There's no files present on the client side in that case.

Yeah, its only about the "possible" UX when a user clicks on the file to view it. It we would provide a "preview" of an image in the future.

@langz langz closed this as completed in ae4648a Dec 13, 2024
tujoworker pushed a commit that referenced this issue Dec 13, 2024
## [10.61.0](v10.60.1...v10.61.0) (2024-12-13)

### 📝 Documentation

* add guidelines for writing commit messages to the contribution guide ([#4389](#4389)) ([34eff0e](34eff0e))
* **Upload:** add `id` property ([#4401](#4401)) ([56d4956](56d4956))

### ✨ Features

* **Forms:** add missing support for `defaultValue` for Field.Slider ([#4394](#4394)) ([701ab66](701ab66))
* **Logo:** update sbanken logo ([#4379](#4379)) ([069ae1a](069ae1a))
* **Upload:** make `id` property as optional ([#4405](#4405)) ([0c58973](0c58973))
* **Value.Upload:** add async `onFileClick` event ([#4397](#4397)) ([be1c21c](be1c21c))

### 🐛 Bug Fixes

* **DatePicker:** add `null` as possible date type as return value ([#4407](#4407)) ([363e0b5](363e0b5))
* **DatePicker:** throw error when `date` is invalid ([#4396](#4396)) ([f977ebc](f977ebc))
* **FieldBlock:** remove max-width for label when width stretch ([#4406](#4406)) ([20c02d4](20c02d4))
* **Forms:** enhance transformIn and transformOut to support changed array and object instances ([#4392](#4392)) ([ae4648a](ae4648a)), closes [#4366](#4366)
* **Slider:** ensure `min` and `max` value is respected when `step` doesn't fit exactly ([#4395](#4395)) ([2c00b0c](2c00b0c))
* **Tabs:** ensure cached content never takes up visual space ([#4399](#4399)) ([360aacc](360aacc))
* **Upload:** display files without anchor when their size is not given ([#4390](#4390)) ([70df7c8](70df7c8))
* **Upload:** display spinner in async `onFileClick` without file `id` ([#4393](#4393)) ([b743d6e](b743d6e))
@tujoworker
Copy link
Member

🎉 This issue has been resolved in version 10.61.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 participants