Open zimeg opened 1 month ago
Oh weird. Yeah that doesn't seem right 😬 Oh wait.. there's a difference between web.files.uploadV2
and web.filesUploadV2
isn't there?
Damn, I don't think I actually audited web.filesUploadV2
at all.. I just looked at the methods within web.files.*
... yeah, I didn't write up any migration instructions for v7 for web.filesUploadV2
, just for web.files.uploadV2
😢
This is likely an oversight by me as part of the work to release web-api v7.
@filmaj no blame at all since this wasn't covered by test! I've even poked this method since that release and didn't catch this until now!
The difference between web.files.uploadV2
and web.filesUploadV2
also surprised me. Without digging into it, it's not so clear to me what web.files.uploadV2
actually maps to 🤔 It might make sense to only support the wrapping web.filesUploadV2
here IMO 😳
If this does seem unexpected, with the top level result.files[ii]
containing actual file information being correct, does it seem right to mark this as a semver:major
fix?
Yes, unfortunately :(
Ahh this might be a nice reminder that versioning signals the type of change being made and nothing more, nothing too deep to read into 😰 This is what I'm telling myself.
Yeah it all depends on the project. web-api doesn't tend to do majors often, so naturally the amount of breaking changes add up over time, so we kind of do it to ourselves.
I steward a different open source project that releases major new versions more frequently - maybe two or three times a year - but the backwards incompatibilities are minor and few and far between that most folks are unaffected by upgrading. Top it off with some good upgrade instructions with each major release and it's not actually that painful or scary.
I compare it to testing and CI: the less often you test, the more changes batch up, the harder it becomes to test - a negative feedback loop.
👋 It looks like this issue has been open for 30 days with no activity. We'll mark this as stale for now, and wait 10 days for an update or for further comment before closing this issue out. If you think this issue needs to be prioritized, please comment to get the thread going again! Maintainers also review issues marked as stale on a regular basis and comment or adjust status if the issue needs to be reprioritized.
Overview
A successful response from uploading files with the
filesUploadV2
API contains afiles
attribute after a request like so:Which means accessing the ID of this file needs:
The first
[0]
index is also consistent when multiple files are uploaded:Expected behaviors
This seems like unexpected behavior so wanted to check - it caught me by surprise! I expected
result.files[0].id
andresult.files[1].id
to have the file IDs 🤔I found the following quick change removes the middle
files[0]
too:https://github.com/slackapi/node-slack-sdk/blob/193840815abf0dff20f28123412ed7cb44558c20/packages/web-api/src/WebClient.ts#L454
Also noticing a few oddities around typing differences between
filesUploadV2
andfiles.uploadV2
but this doesn't seem so important to me iffilesUploadV2
is the recommended method of file uploads (typings work well here!).Packages:
Select all that apply:
@slack/web-api
Reproducible in:
The Slack SDK version
Node.js runtime version
v20.12.2
OS info
Requirements
For general questions/issues about Slack API platform or its server-side, could you submit questions at https://my.slack.com/help/requests/new instead. :bow:
Please read the Contributing guidelines and Code of Conduct before creating this issue or pull request. By submitting, you are agreeing to those rules.