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

Implement File class #39015

Closed
jimmywarting opened this issue Jun 12, 2021 · 16 comments · Fixed by #45139
Closed

Implement File class #39015

jimmywarting opened this issue Jun 12, 2021 · 16 comments · Fixed by #45139
Labels
feature request Issues that request new features to be added to Node.js. fs Issues and PRs related to the fs subsystem / file system.

Comments

@jimmywarting
Copy link

jimmywarting commented Jun 12, 2021

I think a File class would be useful to implement also, it's a quite simple class that only extends Blob with two properties name and lastModified, it dose not have to do much else beside also being transferable.

File is a fundamental building block for aligning with more Web Platform APIs such as FormData, native File System Access and even node:fs itself... the simplast solution to save it being await fsPromises.writeFile(path, blob.stream()) as it supports async iterable streams

A step in appending a blob to FormData is to convert blobs to a File instance (to give it a name) and make it possible to serialize the FormData when you iterate over all entries. Or getting a entry with formData.get(field)
FormData is a desired tool for all those folks who wants to see support for window.fetch in core

fetch-blob just added a File class as well, we saw many ppl extending fetch-blob with a own File class and wanted to remove that burdan for everyone so everybody could use the same instances instead.

Therefore we also added a utility fn that could create a File from a path. fileFrom(path, [mimetype]) and fileFromSync(path, [mimetype]) which i hope to see being implemented into fs one day (#37340).

(in fetch-blob it first create a BlobDataItem (that is similar to what a Blob looks like but don't read anything into memory, it backed up by the filesystem instead) and then wrap it with a File class new File([BlobDataItem], name, { type }))

@jimmywarting
Copy link
Author

jimmywarting commented Jun 12, 2021

I would like to help and make a PR that implements this, but I have never contributed or looked into the code before (write test & documentation, making transferable items and more) but what i could do is to contribute with a File class that i think could be helpful and help review anyone who make a PR

/*  ./node/lib/internal/blob.js */

const kName = Symbol('kName');
const kLastModified = Symbol('kLastModified');

class InternalFile extends JSTransferable {
  constructor(handle, length, name, lastModified, type = '') {
    super([], '');
    this[kHandle] = handle;
    this[kType] = type;
    this[kLength] = length;
    this[kName] = name;
    this[kLastModified] = lastModified;
  }
}

class File extends Blob {
  constructor(fileBits, fileName, options = {}) {
    emitExperimentalWarning('buffer.File');
    if (arguments.length < 2) {
      throw new TypeError(`Failed to construct 'File': 2 arguments required, but only ${arguments.length} present.`);
    }
    super(fileBits, options);

    const modified = Number(options.lastModified);
    this[kLastModified] = Number.isNaN(modified) ? Date.now() : modified;
    this[kName] = fileName;
  }

  get name() { return this[kName]; }

  get lastModified() { return this[kLastModified]; }

  [kClone]() {
    return {
      data: {
        handle: this[kHandle],
        type: this[kType],
        length: this[kLength],
        lastModified: this[kLastModified],
        name: this[kName]
      },
      deserializeInfo: 'internal/file:InternalFile'
    };
  }

  [kDeserialize](data) {
    this[kHandle] = data.handle;
    this[kType] = data.type;
    this[kLength] = data.length;
    this[kLastModified] = data.lastModified;
    this[kName] = data.name;
  }
}

ObjectDefineProperty(File.prototype, SymbolToStringTag, {
  configurable: true,
  value: 'File'
});

InternalFile.prototype.constructor = File;
ObjectSetPrototypeOf(InternalFile.prototype, File.prototype);

@targos
Copy link
Member

targos commented Jun 15, 2021

@jasnell probably has some ideas on this topic.

@targos targos added feature request Issues that request new features to be added to Node.js. fs Issues and PRs related to the fs subsystem / file system. labels Jun 15, 2021
@jasnell
Copy link
Member

jasnell commented Jun 15, 2021

Yeah I've been planning on this implementing this for a while but need to get some other bits done first. The Blob impl, for instance, needs to be finished to allow for this

@jimmywarting
Copy link
Author

jimmywarting commented Feb 22, 2022

undici have also implementing this file class into theirs fetch implementation.

So there is currently a round about way of getting a file class now i suppose?
requires --experimental-fetch + node v17.5

import { Blob } from 'node:buffer'

const { constructor: FormData } = await new Response(new URLSearchParams()).formData()
const fd = new FormData()
fd.set('x', new Blob())
const File = fd.get('x').constructor

new File([], '') instanceof Blob // ???

supposedly their formdata implementation also accepts 3th party blobs look a like items? so is File really a instances of Blob?

@MKRhere
Copy link

MKRhere commented Sep 28, 2022

Since Node vendors undici, it should be a matter of exposing https://github.com/nodejs/node/blob/3d49437262e4365dd25ce6009b41c205f930bcd5/deps/undici/src/lib/fetch/file.js

Am I missing something?

@MKRhere
Copy link

MKRhere commented Sep 28, 2022

From a naive glance, it looks like adding File to this list should suffice:

ObjectDefineProperties(globalThis, {
FormData: lazyInterface('FormData'),
Headers: lazyInterface('Headers'),
Request: lazyInterface('Request'),
Response: lazyInterface('Response'),
});

@jimmywarting
Copy link
Author

The weird thing is that they have 2 kinds of File classes. One that extends native Blob and another File like class that can not be structuredClonable or sent via post message to other workers or with object urls.
The file like class can accept other 3rh party blobs that can be backed up by custom filesystem

@KhafraDev
Copy link
Member

The spec-compliant File could be exposed after nodejs/undici#1687, I don't think there's anything holding it back anymore?

nodejs-github-bot pushed a commit that referenced this issue Nov 10, 2022
PR-URL: #45139
Fixes: #39015
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
ruyadorno pushed a commit that referenced this issue Nov 21, 2022
PR-URL: #45139
Fixes: #39015
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
ThisIsMissEm pushed a commit to ThisIsMissEm/node that referenced this issue Dec 6, 2022
PR-URL: nodejs#45139
Fixes: nodejs#39015
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
danielleadams pushed a commit that referenced this issue Dec 30, 2022
PR-URL: #45139
Fixes: #39015
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
danielleadams pushed a commit that referenced this issue Dec 30, 2022
PR-URL: #45139
Fixes: #39015
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
danielleadams pushed a commit that referenced this issue Jan 3, 2023
PR-URL: #45139
Fixes: #39015
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
danielleadams pushed a commit that referenced this issue Jan 4, 2023
PR-URL: #45139
Fixes: #39015
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@isker
Copy link
Contributor

isker commented Mar 20, 2023

Sorry to ping on this old issue, but is there a reason this wasn't added to the global object in #45139 like Blob and many other web APIs? I can open a new issue for that if it's appropriate.

CC @KhafraDev.

@KhafraDev
Copy link
Member

#47153

@jasnell
Copy link
Member

jasnell commented Mar 20, 2023

@isker ... To answer the specific question: we generally don't add experimental new apis to the global scope until they graduate from experimental.

@isker
Copy link
Contributor

isker commented Mar 20, 2023

#47153

Thanks!

@isker ... To answer the specific question: we generally don't add experimental new apis to the global scope until they graduate from experimental.

Well, scroll down this and observe all the experimental fetch APIs on global 😬.

@jasnell
Copy link
Member

jasnell commented Mar 20, 2023

Yes, I know it hasn't been applied consistently. That's the intent but doesn't always happen. Either way, #47153 should get this one there.

@jasnell
Copy link
Member

jasnell commented Mar 20, 2023

I'd also just stress the new part of that. The experimental APIs that are global currently were not global when they were first introduced.

@jimmywarting
Copy link
Author

but i don't think this 2 should be such a huge blocker. for adding it to the global scope

@KhafraDev
Copy link
Member

KhafraDev commented Mar 20, 2023

Neither of those are issues with File, but with Blob. And that's if you consider 2 an issue when it matches other implementations and the spec isn't clear.

Undici's File class does strictly follow the spec, which seemingly doesn't cause issues, but I don't think so either approach is wrong.

@anonrig anonrig moved this from Pending Triage to Done in Node.js feature requests Mar 22, 2023
Offroaders123 added a commit to Offroaders123/Gamedata-Parser that referenced this issue May 2, 2023
Super happy about this! While working on the cross-platform (in regards to the different Minecraft versions/platforms) Minecraft world APIs, I decided to go back and rework the code for this projectt before moving this existing code over to there. I looked into the status of the File API constructor in NodeJS, as I remembered it being one of the most recent things that would be a great upgrade here, and looks like it just landed about a month ago! Super awesome. That takes one more thing out, and now it's a little bit less of a headache to make work everywhere. That's awesome.

nodejs/node#39015
nodejs/node#47153
nodejs/node@7bc0e6a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants