enable passing optional meta data to Uppy URL plugin's addFile API #3788
Conversation
Uppy Url is great! Thank you. For my use case, I need to be able to pass thru some meta data with the File, in addition to the URL. Harmless to make it optional, but very useful to have it. Without it, I had to copy/paste the code in addFile to add it externally, since there's no intermediate opportunity to add data before uppy.addFile gets called.
one more change - filenames need to have the query string/args stripped from the URL, if present
|
Thanks for the PR. Wouldn't calling uppy.setFileMeta work to add meta? |
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
I don't think so. The reason is sequencing or WHEN. As I mentioned in the PR comments, there's no intermediate opportunity to add data before uppy.addFile gets called. When upp.addFile gets called, there are registered listeners that receive the File object BEFORE I have a chance to add meta data to it. |
There was a problem hiding this comment.
I don't think so. The reason is sequencing or WHEN. As I mentioned in the PR comments, there's no intermediate opportunity to add data before uppy.addFile gets called.
This seems to be an XY problem. This is the solution but why is it a problem? Can you describe your use case better? I have a feeling we don't need this.
Also this is the internal addFile of the plugin, normally you would use uppy.addFile for programmatic usages.
|
In our code, we have registered listeners for the 'files-added' event, e.g uppy.on('files-added', ... Our handler is called when addFiles is called. This happens BEFORE addFiles returns. Our handler wants the file to receives to have some meta data. The Uppy URL plugin has only 1 API: addFile: so if registered handlers of the 'files-added' want a file with meta data, there is currently no opportunity to add that data. Our current workaround is that we've copy pasted the code from Uppy URL so that we However, given that the plugin has only 1 API and we had to copy/paste its implementation, we are It's because addFile dispatches the event before it returns that there is no current solution, if you want to use Uppy URL without copy/pasting it's internals. Hope that helps clarify the problem. If you see a way that the Uppy URL addFile API can pass meta data to 'files-added' listeners as is, please let us know. |
Co-authored-by: Merlijn Vos <merlijn@soverin.net>
getFileNameFromUrl change is now part of separate PR
bradedelman
changed the title
|
split the change to url -> filename into separate PR |
|
added the change to the typescript wrapper |
I think this is what I meant with the XY problem. The problem seems to be that you can't do X in the It's a trend that people want to to control X at Y point in time and it means adding endless options. While sometimes there is a different workaround with existing options. |
Thank you, @bradedelman! |
|
I view it differently. uppy.addFiles is an existing API. it allows setting meta data. Uppy URL addFile wraps it, but limits access. By allowing passing in meta data, it simply exposes what would appear to be the design-intent of addFile. As such, I want to use addFile as it was intended, but the Uppy URL plugin doesn't expose. that said, yes, if there was a chance to modify the File object before the "files-added" listeners were called, that would also be fine. Given that Files have a meta data field, I'm really uncertain why you seem opposed to us using it. Not clear why we need to justify our use to you, given the design intent of Uppy is to allow it. |
|
After investigation I personally think you are right. With |
|
exactly! thanks for validating my point. hoping we can build consensus. |
There was a problem hiding this comment.
Given that Files have a meta data field, I'm really uncertain why you seem opposed to us using it. Not clear why we need to justify our use to you, given the design intent of Uppy is to allow it.
I'm just trying to verify anything that extends to API surface even further. Uppy and its packages have a lot of options, events, and methods. We get requests to extend the API regularly, but a product can't be everything and often a well designed set of primitives should cover for all cases within the product philosophy. I wasn't inherently against this change.
Thanks for explaining everything in detail. I think we can indeed move ahead with this change :)
per PR review suggestion, improve the type of the new optional argument
|
All good if tests pass |
|
thank you everyone for the review and improvements. |
Thank you for your contribution and for your patience during this review :) |
|
:-) will it be released as uppy@2.12.0 or? |
|
Generally once a month, and for important bug fixes we release whenever. We are currently busy with 3.0-beta releases but can probably squeeze a regular one out soon |
|
that'd be great. |
| Package | Version | Package | Version | | ---------------------- | ------- | ---------------------- | ------- | | @uppy/aws-s3 | 2.2.1 | @uppy/tus | 2.4.1 | | @uppy/aws-s3-multipart | 2.4.1 | @uppy/url | 2.2.0 | | @uppy/companion-client | 2.2.1 | @uppy/xhr-upload | 2.1.2 | | @uppy/core | 2.3.1 | @uppy/robodog | 2.8.0 | | @uppy/react | 2.2.2 | uppy | 2.12.0 | | @uppy/remote-sources | 0.1.0 | | | - @uppy/remote-sources: Add @uppy/remote-sources preset/plugin (Artur Paikin / #3676) - @uppy/react: Reset uppy instance when React component is unmounted (Tomasz Pęksa / #3814) - @uppy/aws-s3-multipart,@uppy/aws-s3,@uppy/tus: queue socket token requests for remote files (Merlijn Vos / #3797) - @uppy/xhr-upload: replace `ev.target.status` with `xhr.status` (Wes Sankey / #3782) - @uppy/core: fix `TypeError` when file was deleted (Antoine du Hamel / #3811) - @uppy/robodog: fix linter warnings (Antoine du Hamel / #3808) - meta: fix GHA workflow for prereleases (Antoine du Hamel) - @uppy/aws-s3-multipart: allow `companionHeaders` to be modified with `setOptions` (Paulo Lemos Neto / #3770) - @uppy/url: enable passing optional meta data to `addFile` (Brad Edelman / #3788) - @uppy/url: fix `getFileNameFromUrl` (Brad Edelman / #3804) - @uppy/tus: make onShouldRetry type optional (Merlijn Vos / #3800) - doc: fix React examples (Antoine du Hamel / #3799) - meta: add GHA workflow for prereleases (Antoine du Hamel)
Uppy Url is great! Thank you. For my use case, I need to be able to pass thru some meta data with the File, in addition to the URL. Harmless to make it optional, but very useful to have it. Without it, I had to copy/paste the code in addFile to add it externally, since there's no intermediate opportunity to add data before uppy.addFile gets called.