-
Notifications
You must be signed in to change notification settings - Fork 112
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
Add support for Web File on Deno #28
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -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] | ||
private [internalInputFileData]: ConstructorParameters<typeof InputFile>[0] | ||
public get [inputFileData]() { | ||
const file = this[internalInputFileData] | ||
return file instanceof Blob ? file.stream() : file | ||
} | ||
Comment on lines
+51
to
+55
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know what I dislike about this change. This logic should be moved into There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. InputFile is semantically part of the client. The two cannot be separated. It just has to reside in a different file for cross-platform reasons. In a way, the InputFile encapsulates the runtime-specific part of the client. Given that calling That being said, I know this way to divide the code is mediocre at best. Maybe d2n will improve the situation. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. d2n should be able to shim There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Node 16.7.0 added |
||
/** | ||
* Optional name of the constructed `InputFile` instance. | ||
* | ||
|
@@ -66,12 +72,14 @@ export class InputFile { | |
| string | ||
| Uint8Array | ||
| ReadableStream<Uint8Array> | ||
| AsyncIterable<Uint8Array>, | ||
| AsyncIterable<Uint8Array> | ||
| Blob, | ||
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 | ||
} | ||
} | ||
|
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.
For 2.0,
InputFile
should acceptBlob
instead ofpath: string
(nodejs/node#37340), and expose justname: string
and a.stream()
method.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.
That sounds like a good idea, that would simply the abstraction to the client 👍🏻
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.
Further, you could turn
InputFile
into an interface, and provideclass InputFileAsyncIterable
instead, perhaps some others. Zero type-switching in your code!You could still easily detect
InputFile
s withNote that web
File
already satisfiesThere 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.
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 theInputFile
constructor.That being said, I'm open to change completely how
InputFile
and client work together if there is a better solution.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.
I'd argue that
new upload.Buffer(uint8array, name)
ornew upload.Iterable(streamOrIterable, name)
orawait Deno.getFile(path)
is more explicit thannew InputFile(oneOfFourTypes)
.I don't think it makes sense to ever merge this PR. Better open a fresh one when working on 2.0.
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.
On 2nd thought, this could be made mergable:
deno2node
,Blob
,Blob
detection toClient
.But there doesn't seem to be any demand.