-
-
Notifications
You must be signed in to change notification settings - Fork 34.2k
buffer: graduate File from experimental and expose as global #47153
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
Conversation
|
Review requested:
|
|
@nodejs/tsc @nodejs/buffer Can you review please? |
cdb81c1 to
fb57970
Compare
mcollina
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
GeoffreyBooth
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does its API match the browser counterpart? Assuming it does, this seems fine to me.
yes it's a 1:1 match |
Commit Queue failed- Loading data for nodejs/node/pull/47153 ✔ Done loading data for nodejs/node/pull/47153 ----------------------------------- PR info ------------------------------------ Title buffer: graduate File from experimental and expose as global (#47153) Author Khafra (@KhafraDev) Branch KhafraDev:make-file-global -> nodejs:main Labels semver-major, author ready, needs-ci, commit-queue-squash Commits 2 - buffer: graduate File from experimental and expose as global - doc: apply suggestions from code review Committers 1 - Khafra <42794878+KhafraDev@users.noreply.github.com> PR-URL: https://github.com/nodejs/node/pull/47153 Reviewed-By: Yagiz Nizipli Reviewed-By: Matteo Collina Reviewed-By: Geoffrey Booth Reviewed-By: Robert Nagy ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/47153 Reviewed-By: Yagiz Nizipli Reviewed-By: Matteo Collina Reviewed-By: Geoffrey Booth Reviewed-By: Robert Nagy -------------------------------------------------------------------------------- ℹ This PR was created on Sat, 18 Mar 2023 21:34:05 GMT ✔ Approvals: 4 ✔ - Yagiz Nizipli (@anonrig): https://github.com/nodejs/node/pull/47153#pullrequestreview-1349416220 ✔ - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/47153#pullrequestreview-1349430849 ✔ - Geoffrey Booth (@GeoffreyBooth) (TSC): https://github.com/nodejs/node/pull/47153#pullrequestreview-1349668541 ✔ - Robert Nagy (@ronag) (TSC): https://github.com/nodejs/node/pull/47153#pullrequestreview-1350351079 ✘ Last GitHub CI failed ℹ Last Full PR CI on 2023-03-21T12:31:57Z: https://ci.nodejs.org/job/node-test-pull-request/50514/ - Querying data for job/node-test-pull-request/50514/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/4482731187 |
aduh95
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should remove the following in a follow up PR:
Lines 7 to 10 in 11e37e0
| runner.setInitScript(` | |
| const { File } = require('buffer'); | |
| globalThis.File = File; | |
| `); |
|
Landed in 7bc0e6a |
This comment was marked as abuse.
This comment was marked as abuse.
This comment was marked as abuse.
This comment was marked as abuse.
|
We don't implement HTML forms https://wicg.github.io/entries-api/#dom-file-webkitrelativepath |
Undici has been using the native File class by default for over 3 months now, and we haven't had a single issue reported about it or using it with fetch/FormData. It's a very small wrapper around Blob too, I don't personally see much reason in keeping a class with 2 additional properties experimental.
I based this commit heavily on jasnell's which made Blob global, lmk if I'm missing something