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

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

Merged

Conversation

mbuella
Copy link
Contributor

@mbuella mbuella commented Jan 9, 2025

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
Copy link
Collaborator

adamziel commented Jan 9, 2025

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
Copy link
Contributor Author

mbuella commented Jan 10, 2025

@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
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
9 of 10 checks passed
@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
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants