-
Notifications
You must be signed in to change notification settings - Fork 120
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
Support creating InputFile instances from URLs #80
Conversation
What's the benefit of using |
Those are two very different things. What are you referring to? |
} | ||
|
||
async function* fetchFile(url: string | URL): AsyncIterable<Uint8Array> { | ||
const { body } = await fetch(url); |
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.
is fetch
global? i don't see where it come from
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.
Only on Deno and the web. On Node, it is shimmed by deno2node using node-fetch.
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.
Maybe it should be improved in deno2node that the shimming isn't performed for Node-specific source files.
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 don't know anything about deno2node. Just looking at the file as a whole from Node.js dev perspective i see fetch
that is not imported from anywhere so it will crash until fetch
is added to globals
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 see you asked in deno2node project to remove shimming in node specific files, so maybe you should add the fetch
import then?
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 can only do that after the changes to deno2node. Right now, the shimming will produce clashing imports, hence causing the compilation to fail.
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.
Oh, I thought the shim would add it to globals, ok then :)
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.
Ah I see. No, shimming in deno2node works by injecting import statements into the modules that actually need the shim. It does not touch global definitions.
I wrote a new README in wojpawlik/deno2node#14 a long time ago but it has not been merged yet. I improved this README now with respect to this misunderstanding. I hope we have a chance to get this through soon
cc @wojpawlik
if (filename === undefined && typeof file === "string") { | ||
filename = basename(file); | ||
} | ||
this.filename = filename; | ||
} | ||
get [inputFileData]() { | ||
if (this.consumed) { |
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 looked at this PR again and:
- won't this break
auto-retry
? I mean the plugin won't be able to actually retry the request, because InputFile will be always consumed. - I don't see
this.consumed = true
anywhere
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.
won't this break
auto-retry
?
No, because the consumed
flag is only meant to be set when iterators or other non-reusable data sources are used.
I don't see
this.consumed = true
anywhere
True. This was fixed in 23dcf2a
(#77).There is a lot going on in the same files right now, and I agree that this get a little difficult.
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 think the problem was that we should not have merged this before #77, as I stated in the original description.
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.
Doesn't GitHub have something to block merges before something else is merged?
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.
"PR dependencies" or something like that. I'm not a heavy user of GitHub, so I don't really know
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.
No, but I could have specified a target branch other than main
. That means we basically first integrate one PR into the other, and then the merged result into main
.
Adds the following syntax for constructing
InputFile
s based on remote data.The data will be fetched lazily from the specified URL as soon as the
InputFile
is sent. The file contents are streamed, so only a small portion of the data is kept in memory at all times. When several files are sent in an album, they are piped though one by one, such that only a single outgoing request is open at all times.Currently does not support more advanced configuration options. Note that it's already possible to do this:
This allows you to pass any configuration to
fetch
.Currently does not support caching files on disk. This is very hard to get right, and it is questionable if it has any advantages. Confer the discussion at #33 for more information about caching.
Based off of #77. Please review and merge #77 first.
Closes #33.