Skip to content
This repository was archived by the owner on Jan 23, 2026. It is now read-only.

Improve uploadOrUpdate returned data. - #63

Merged
alaister merged 11 commits into
supabase:nextfrom
mircea-pavel-anton:fix/upload
Jul 4, 2022
Merged

Improve uploadOrUpdate returned data.#63
alaister merged 11 commits into
supabase:nextfrom
mircea-pavel-anton:fix/upload

Conversation

@mircea-pavel-anton

Copy link
Copy Markdown
Contributor

Fixes #9

What is the current behavior?

Currently, the uploadOrUpdate function (so automatically the upload and update functions as well), only returns a Key: value pair that represents the file path prefixed by the bucket id.

This path cannot be then passed to the download method as the download doesn't need the bucket id prefix.

What is the new behavior?

By simply adding 2 more Key: value pairs to the returned data object, we can provide more useful information in the returned object:

{
  Key: the old path, for backwards compatibility
  downloadPath: the path not prefixed by the bucket ID
  bucketId: the id of the bucket
}

By also providing the bucket ID in the returned data, we give users the full set on information required to be able to call download, as I assume that the data returned from upload will then immediately be stored in the database for later use.

By implementing these changes, we enable a workflow similar to this:

const { data, error } = await storageClient.from('bucket').upload('/folder/file.txt', fileBody)
// ...
const { data2, error2 } = await storageClient.from(data.bucketId).download(data.downloadPath)

@alaister alaister left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks again @mirceanton!

What do you think about calling downloadPath just path? A nit, I know, but I think it's a little cleaner :)

Also, if you fancy adding a test or two for file upload & update, that would be greatly appreciated!

Comment thread README.md Outdated
```

> Note:
> The path in `data.key` is prefixed by the bucket ID and is not the value which should be passed to the `download` method in order to fetch the file.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should be data.Key

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

nice catch 👍🏻

@mircea-pavel-anton

Copy link
Copy Markdown
Contributor Author

Yep, i think simply calling it "path" is good. I will look into tests, sure.

@mircea-pavel-anton

Copy link
Copy Markdown
Contributor Author

@alaister sorry I've been inactive for such a long time on this one. I've been rather busy lately. I fixed the mentioned issues, but I don't think I will get around to making some proper tests until this summer (around July perhaps). I've my bachelor degree taking up all of my spare time till then. 😅

@mircea-pavel-anton

Copy link
Copy Markdown
Contributor Author

As a side note, something that I noticed is that running npm build fails on the build::umd task with the following output on my machine:

mike@workstation:~/workspace/supabase/storage-js$ npm run build

> @supabase/storage-js@0.0.0 build
> genversion src/lib/version.ts --es6 && run-s clean format build:*


> @supabase/storage-js@0.0.0 clean
> rimraf dist docs


> @supabase/storage-js@0.0.0 format
> prettier --write "{src,test}/**/*.ts"

src/index.ts 153ms
src/lib/constants.ts 10ms
src/lib/fetch.ts 53ms
src/lib/helpers.ts 10ms
src/lib/index.ts 4ms
src/lib/StorageBucketApi.ts 44ms
src/lib/StorageFileApi.ts 92ms
src/lib/types.ts 14ms
src/lib/version.ts 6ms
src/StorageClient.ts 6ms
test/storageApi.test.ts 12ms
test/storageFileApi.test.ts 5ms

> @supabase/storage-js@0.0.0 build:main
> tsc -p tsconfig.json


> @supabase/storage-js@0.0.0 build:module
> tsc -p tsconfig.module.json


> @supabase/storage-js@0.0.0 build:umd
> webpack

/home/mike/workspace/supabase/storage-js/node_modules/loader-runner/lib/LoaderRunner.js:146
                if(isError) throw e;
                            ^

Error: error:0308010C:digital envelope routines::unsupported
    at new Hash (node:internal/crypto/hash:67:19)
    at Object.createHash (node:crypto:135:10)
    at BulkUpdateDecorator.hashFactory (/home/mike/workspace/supabase/storage-js/node_modules/webpack/lib/util/createHash.js:144:18)
    at BulkUpdateDecorator.update (/home/mike/workspace/supabase/storage-js/node_modules/webpack/lib/util/createHash.js:46:50)
    at RawSource.updateHash (/home/mike/workspace/supabase/storage-js/node_modules/webpack-sources/lib/RawSource.js:64:8)
    at NormalModule._initBuildHash (/home/mike/workspace/supabase/storage-js/node_modules/webpack/lib/NormalModule.js:839:17)
    at handleParseResult (/home/mike/workspace/supabase/storage-js/node_modules/webpack/lib/NormalModule.js:904:10)
    at /home/mike/workspace/supabase/storage-js/node_modules/webpack/lib/NormalModule.js:995:4
    at processResult (/home/mike/workspace/supabase/storage-js/node_modules/webpack/lib/NormalModule.js:718:11)
    at /home/mike/workspace/supabase/storage-js/node_modules/webpack/lib/NormalModule.js:778:5 {
  opensslErrorStack: [ 'error:03000086:digital envelope routines::initialization error' ],
  library: 'digital envelope routines',
  reason: 'unsupported',
  code: 'ERR_OSSL_EVP_UNSUPPORTED'
}

Node.js v17.8.0
ERROR: "build:umd" exited with 1.
mike@workstation:~/workspace/supabase/storage-js$ 

I get this both on this branch and the main branch.

@alaister

Copy link
Copy Markdown
Member

Thanks for the fixes @mirceanton. No worries at all, we've been busy too!

We're aiming to get all these PRs released by the end of this month, so I'll merge this in soon.

If you do find some time in the summer to write some tests, contributions are always welcome! (not a problem if not, though 🙂)

Thanks for the report on the umd build too, I'll check it out.

@alaister
alaister changed the base branch from main to next June 29, 2022 07:01
@alaister

Copy link
Copy Markdown
Member

Hey @mirceanton,
Just circling back around to this now.

I've updated this to go into the next branch, would you mind fixing the merge conflicts and updating the return types to match how we're doing returns now?

Let me know if you run into anything or don't have the bandwidth to make these changes at the moment.

Thanks!

@alaister alaister mentioned this pull request Jun 29, 2022
@mircea-pavel-anton

Copy link
Copy Markdown
Contributor Author

Hey @alaister ! I will look into fixing things up by the end of this week, no worries!

@mircea-pavel-anton

Copy link
Copy Markdown
Contributor Author

Hmm, I updated my branch and then did a rebase on the next branch and it seems there are a few commits now that are outside the scope of this PR. Should I drop them? @alaister

Comment thread src/lib/StorageFileApi.ts
Comment on lines +65 to +74
): Promise<
| {
data: { Key: string; path: string; bucketId?: string }
error: null
}
| {
data: null
error: StorageError
}
> {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@alaister is this what you meant by " updating the return types to match how we're doing returns now"?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes perfect 👌

@alaister alaister left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @mirceanton! Looks great!

Don't worry about the commits outside the scope of this PR, they needed to go into next anyway so they're fine here.

@alaister
alaister merged commit 6c94809 into supabase:next Jul 4, 2022
@mircea-pavel-anton
mircea-pavel-anton deleted the fix/upload branch July 4, 2022 08:49
@kiwicopple

Copy link
Copy Markdown
Member

🎉 This PR is included in version 1.8.0-next.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@kiwicopple

Copy link
Copy Markdown
Member

🎉 This PR is included in version 2.0.0-rc.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

alaister added a commit that referenced this pull request Oct 11, 2022
* feat: improve function return types (#60)

* feat: improve types

* chore: better error handling

* feat: custom storage api error

* chore: use custom error type

* chore: update custom error type

* chore: replace instanceof with isStorageError

* chore: move isStorageError to a non-static method

* Improve `uploadOrUpdate` returned data. (#63)

* fix: typo in updateBucket jsdoc

* fix: import cross-fetch conditionally

* fix: es2020

* Also return the clean path for the uploaded file.

* Update readme.

* Add bucket id in returned data.

* Update readme

* Fix return type for `upload` and `update`.

* Fix typo in code snippet comment.

* Rename `downloadPath` to `path`

* Update return types

Co-authored-by: Jonathan Picques <jonathan.picques@gmail.com>
Co-authored-by: Bobbie Soedirgo <bobbie@soedirgo.dev>
Co-authored-by: Mircea-Pavel Anton <mircea.anton99@yahoo.com>

* Improve `getPublicUrl` (#62)

* Improve `getPublicUrl`

* Make return types consistent across all functions.

* Undo return type change

* Change the `getPublicUrl` method to return only the url as string.

* Remove redundant method.

* Update tests.

Co-authored-by: Mircea-Pavel Anton <mircea.anton99@yahoo.com>

* upload, update: don't return bucketId

as a return parameter and in the path. Just returning the path without the bucket id makes it easier to pass the value in to other storage-js functions. Also not calling it Key -- was uppercase and not as clear as just calling it path.

* add docs folder to .gitignore

* Merge branch 'master' into chore/merge-master-04-08-22 (#82)

* fix: remove release config from package.json (#83)

* chore: merge main into next (#85)

* fix: typo in updateBucket jsdoc

* fix: import cross-fetch conditionally

* fix: es2020

* chore: add search param to SearchOptions jsdoc on list function (#59)

* build(release-next): sets up the next branch as an npm prerelease (#80)

* fix: rename main release branch (#84)

Co-authored-by: Jonathan Picques <jonathan.picques@gmail.com>
Co-authored-by: Bobbie Soedirgo <bobbie@soedirgo.dev>

* fix: encode all urls

fixes #78

this should ideally be done in the api server, but doing this breaking change at the client library first, so when we do it at the backend, only folks who are using the api directly will need to upgrade.

* return values is always wrapped by data

signedURL used to return a url directly and inside the data object. This is inconsistent. Now we always return values inside a data object only.

* fix: pass through all values returned by backend

instead of cherry picking only name - we still do this in the types though.

also ensures data is always an object.

Fixes #6

* fix tests

* fix metadata type

Previously accessing metada.contentType would throw a type error since we set the type to an empty object.

* only export StorageClient

#46

removing the change we added for backward compatibility

* update ci to node 16

* upgrade typedoc to latest version

* fix: move bucket and file api to package folder

docs look better when we do this and add the packages to the entrypoint to the typedoc command

* modify publicurl to always return data

all methods return data and an error

* dont return error for getPublicUrl

* fix: signed url is returned as signedUrl

matches the method name createSignedUrl

* downgrade typedoc to 0.22.16

our doc generation pipeline only works with 0.22.16 for now

* exclude protected properties from typedoc

* fix: typedocs

* chore: merge main into next (#99)

* fix: typo in updateBucket jsdoc

* fix: import cross-fetch conditionally

* fix: es2020

* chore: add search param to SearchOptions jsdoc on list function (#59)

* build(release-next): sets up the next branch as an npm prerelease (#80)

* fix: rename main release branch (#84)

* build(release-rc): sets up the rc branch as an npm prerelease (#98)

Co-authored-by: Jonathan Picques <jonathan.picques@gmail.com>
Co-authored-by: Bobbie Soedirgo <bobbie@soedirgo.dev>

* feat: Release V2 RC

BREAKING CHANGE: Release V2

* update docs (#102)

* Fix docs typo (#104)

* chore: increased test coverage (#106)

* fix: consistent return types for copy (#110)

* feat: download file via url (#112)

* fix: merge main into rc (#113)

* fix: typo in updateBucket jsdoc

* fix: import cross-fetch conditionally

* fix: es2020

* chore: add search param to SearchOptions jsdoc on list function (#59)

* build(release-next): sets up the next branch as an npm prerelease (#80)

* fix: rename main release branch (#84)

* build(release-rc): sets up the rc branch as an npm prerelease (#98)

* Adds v1 docs (#107)

* docs: ci

Co-authored-by: Jonathan Picques <jonathan.picques@gmail.com>
Co-authored-by: Bobbie Soedirgo <bobbie@soedirgo.dev>
Co-authored-by: Copple <10214025+kiwicopple@users.noreply.github.com>

Co-authored-by: Mircea-Pavel Anton <contact@mirceanton.com>
Co-authored-by: Jonathan Picques <jonathan.picques@gmail.com>
Co-authored-by: Bobbie Soedirgo <bobbie@soedirgo.dev>
Co-authored-by: Mircea-Pavel Anton <mircea.anton99@yahoo.com>
Co-authored-by: Inian <inian1234@gmail.com>
Co-authored-by: Fabrizio <fabri.feno@gmail.com>
Co-authored-by: Copple <10214025+kiwicopple@users.noreply.github.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Document Return Value for Upload

5 participants