Skip to content

Rewrote fileToUint8Array function to be also NodeJS/Deno compatible. - #2117

Merged
adamziel merged 2 commits into
WordPress:trunkfrom
mbuella:fileToUint8Array-using-File-arraybuffer
Jan 10, 2025
Merged

Rewrote fileToUint8Array function to be also NodeJS/Deno compatible.#2117
adamziel merged 2 commits into
WordPress:trunkfrom
mbuella:fileToUint8Array-using-File-arraybuffer

Conversation

@mbuella

@mbuella mbuella commented Jan 9, 2025

Copy link
Copy Markdown
Contributor

Motivation for the change, related issues

Performing multipart upload and posting of data currently works on the web (client) side. Doing it on PHP WASM instance running on NodeJS or other server runtimes (except Deno) causes this issue due to FileReader not supported on the runtime:

ReferenceError: FileReader is not defined
    at file:///c/Users/alonbuella/Documents/projects/php-wasm-vercel/node_modules/@php-wasm/universal/index.js:1717:15
    at new Promise (<anonymous>)
    at fileToUint8Array (file:///c/Users/alonbuella/Documents/projects/php-wasm-vercel/node_modules/@php-wasm/universal/index.js:1716:10)
    at encodeAsMultipart (file:///c/Users/alonbuella/Documents/projects/php-wasm-vercel/node_modules/@php-wasm/universal/index.js:1702:38)
    at PHPRequestHandler.ce (file:///c/Users/alonbuella/Documents/projects/php-wasm-vercel/node_modules/@php-wasm/universal/index.js:2070:48)
    at PHPRequestHandler.le (file:///c/Users/alonbuella/Documents/projects/php-wasm-vercel/node_modules/@php-wasm/universal/index.js:2056:33)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)

Implementation details

This PR introduces an alternative to using FileReader in order to determine the Uint8Array value for a File instance. This is creating a new instance of Uint8Array from the file's buffer instead.

File's arrayBuffer instance method and Uint8Array class are both supported on NodeJS and Deno, and as well as on most recent browsers.

Testing Instructions (or ideally a Blueprint)

  • Tested locally via npm run test
  • CI
@mbuella
mbuella force-pushed the fileToUint8Array-using-File-arraybuffer branch 2 times, most recently from 839d183 to ab47f97 Compare January 9, 2025 12:38
@mbuella
mbuella marked this pull request as ready for review January 9, 2025 13:04
@adamziel

adamziel commented Jan 9, 2025

Copy link
Copy Markdown
Collaborator

Really good spot and a great fix, thank you! My only ask would be to add either a test or an inline comment to inform the next person to avoid using the FileReader class and why.

@adamziel adamziel added [Type] Bug An existing feature does not function as intended [Feature] PHP.wasm labels Jan 9, 2025
@mbuella
mbuella force-pushed the fileToUint8Array-using-File-arraybuffer branch from ab47f97 to 1937bf7 Compare January 10, 2025 02:02
@mbuella

mbuella commented Jan 10, 2025

Copy link
Copy Markdown
Contributor Author

@adamziel thanks for your nice feedback! I have added an inline comment to the updated code to explain why we must avoid using FileReader.

@mbuella
mbuella force-pushed the fileToUint8Array-using-File-arraybuffer branch from 1937bf7 to 34129e7 Compare January 10, 2025 09:57
@adamziel

Copy link
Copy Markdown
Collaborator

Thank you @mbuella! The failing test is just a flaky test. Let's merge!

@adamziel
adamziel merged commit 7a40bbd into WordPress:trunk Jan 10, 2025
@mbuella
mbuella deleted the fileToUint8Array-using-File-arraybuffer branch January 10, 2025 11:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Feature] PHP.wasm [Type] Bug An existing feature does not function as intended

2 participants