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.

Sign up for GitHub

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

Jump to bottom

Add support for Web File on Deno #28

Closed
wants to merge 3 commits into from
Closed

Add support for Web File on Deno #28

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d366a05
Add support for Web File on Deno
KnorpelSenf Aug 15, 2021
55b064d
Permit Blob objects
KnorpelSenf Aug 15, 2021
99304fa
Merge branch 'main' into web-file-support
KnorpelSenf Aug 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 13 additions & 5 deletions deno/src/platform.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ export const baseFetchConfig = {}
// === InputFile handling and File augmenting
// Accessor for file data in `InputFile` instances
export const inputFileData = Symbol('InputFile data')
// Internal data storage for data in `InputFile` instances
const internalInputFileData = Symbol('Internal InputFile data')

/**
* An `InputFile` wraps a number of different sources for [sending
Expand All @@ -46,7 +48,11 @@ export const inputFileData = Symbol('InputFile data')
* Reference](https://core.telegram.org/bots/api#inputfile).
*/
export class InputFile {
public readonly [inputFileData]: ConstructorParameters<typeof InputFile>[0]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For 2.0, InputFile should accept Blob instead of path: string (nodejs/node#37340), and expose just name: string and a .stream() method.

Copy link
Member Author

@KnorpelSenf KnorpelSenf Sep 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds like a good idea, that would simply the abstraction to the client 👍🏻

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Further, you could turn InputFile into an interface, and provide class InputFileAsyncIterable instead, perhaps some others. Zero type-switching in your code!

You could still easily detect InputFiles with

function isInputFile(value: any): value is InputFile {
  return value != null && typeof value.stream === 'function'
}

Note that web File already satisfies

interface InputFile {
  readonly name: string
  readonly stream: () => ReadableStream
}

Copy link
Member Author

@KnorpelSenf KnorpelSenf Sep 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, the goal is to be explicit about file uploads by constructing an InputFile object. When bot code is read, it should be immediately obvious that a file upload will be performed. Permitting lots of things in the API directly contradicts this because it allows to circumvent the InputFile constructor.

That being said, I'm open to change completely how InputFile and client work together if there is a better solution.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd argue that new upload.Buffer(uint8array, name) or new upload.Iterable(streamOrIterable, name) or await Deno.getFile(path) is more explicit than new InputFile(oneOfFourTypes).

I don't think it makes sense to ever merge this PR. Better open a fresh one when working on 2.0.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On 2nd thought, this could be made mergable:

  1. Switch to deno2node,
  2. Shim Blob,
  3. Move Blob detection to Client.

But there doesn't seem to be any demand.

private [internalInputFileData]: ConstructorParameters<typeof InputFile>[0]
public get [inputFileData]() {
const file = this[internalInputFileData]
return file instanceof File ? file.stream() : file
KnorpelSenf marked this conversation as resolved.
Show resolved Hide resolved
}
/**
* Optional name of the constructed `InputFile` instance.
*
Expand All @@ -66,12 +72,14 @@ export class InputFile {
| string
| Uint8Array
| ReadableStream<Uint8Array>
| AsyncIterable<Uint8Array>,
| AsyncIterable<Uint8Array>
| File,
KnorpelSenf marked this conversation as resolved.
Show resolved Hide resolved
filename?: string
) {
this[inputFileData] = file
if (filename === undefined && typeof file === 'string')
filename = basename(file)
this[internalInputFileData] = file
if (filename === undefined)
if (typeof file === 'string') filename = basename(file)
else if (file instanceof File) filename = file.name
this.filename = filename
}
}
Expand Down