Improve `getPublicUrl` #62

mircea-pavel-anton · 2022-04-08T07:28:59Z

Fixes #53 and #27

What is the current behavior?

Currently, getPublicUrl is looking like this:

getPublicUrl(
  path: string
): {
  data: { publicURL: string } | null
  error: Error | null
  publicURL: string | null
} {
  try {
    const _path = this._getFinalPath(path)
    const publicURL = `${this.url}/object/public/${_path}`
    const data = { publicURL }
    return { data, error: null, publicURL }
  } catch (error) {
    return { data: null, error, publicURL: null }
  }
}

Which is not ideal because:

The try... catch doesn't really make much sense as all that happens inside the function, at least to my understanding, is string concatenation of values that are not null thanks to typescript.
The error will always be null.
The return type is inconsistent with the other methods. They all return {data, error} and this would be the only one to return {{data_contents}, error, data_contents}.

What is the new behavior?

My suggestion is to:

Remove the try catch from the function
Change the return type to {data, error} to match the other methods
Create an additional method, something like getPublicUrlAsString that would only return a string of the public url.

This way, we provide backwards compatibility by leaving the old method in place (though removing the publicUrl from the return type may be questionable in this regard, so we may have to leave it in) and we also provide a better getPublicUrl method from now on that simply returns the string.

Additional context

Add any other context or screenshots.

mircea-pavel-anton · 2022-04-08T07:47:02Z

So the CI tests are failing because i removed the publicURL from getPublicURL(). I will add it back so the tests pass and we keep backwards compatibility, but I think that keeping the getPublicUrlAsString or something similar is still worth it and maybe we can migrate to something like that in the next breaking release. Opinions?

alaister

Thanks for your PR @mirceanton!

alaister · 2022-04-08T15:38:40Z

src/lib/StorageFileApi.ts

+    const _path = this._getFinalPath(path)
+    const publicURL = `${this.url}/object/public/${_path}`
+    const data = { publicURL }
+    return { data, error: null, publicURL }


I think you're right in saying this is solved mainly in #60. While it is cleaner to do without the try/catch, with the new custom error type added in #60, it is nice for proper type inference and code consistency.

On second thought I am torn on this one. It would be very smooth to just do:

const { publicURL } = storage.from('my-bucket').getPublicUrl('profile.png')

I don't think simply adding a dummy error field to a function to ensure code consistency is really a good idea. That error will never be anything but null, at least based on my understanding and my testing.

It's not all about ease of use here, but about confusion and introducing bloat in the code base of users. Granted, I am making this seem more of an issue than it actually is. An extra if statement in the code will not degrade performance in any meaningful way but i just don't see the reason behind having the error there since it will never, at least in the current code base, reach an error.

Yep, I get where you're coming from. If not consistency then just for not introducing breaking changes. The new TS type will also show it always being null anyway and we can just mark it as deprecated.

alaister · 2022-04-08T15:47:07Z

src/lib/StorageFileApi.ts

+  getPublicUrlAsString(path: string): string {
+    return this.getPublicUrl(path).data.publicURL
  }


If we're leaving the error clause in getPublicUrl, this method won't work without an error check.
How painful would you find it to continue having to do the error check in your code?
Ideally, I'd like to keep the method the same for less of a breaking change, but if it's ruining the DX for you'd I'd like to know :)

It's not that it's ruining my experience, or that I can't do the error check in my code. It's just that I think the error check is not even required because that method will never throw an error. All it is doing is string concatenation. It doesn't even check if the given path is valid or anything.

For example, the following code works without any errors:

const {data, error} = storageClient.from('pikachu').getPublicUrl("old macdonald had a farm"); console.log( data ); console.log( error );

And i get the error as null and the data as { publicURL: 'https://####################.supabase.co/storage/v1/object/public/pikachu/old macdonald had a farm' }.

So what really is the point of any error checking here, or even of returning the error?

Needless to say, in the above example no such bucket nor file exist in my supabase project.

I think either the method should check that the bucket and file exist and then throw an error if they don't, or just not return any error at all.

I'm inclined to agree with you. Although, I like just removing the try/catch stuff and just having:

const { publicURL } = storage.from('my-bucket').getPublicUrl('profile.png')

no need for adding extra methods, then.

Let's leave this for the weekend and merge it in after #60 gets sorted :)

I suggested adding a new method simply for backwards compatibility. I agree that replacing the current method would be a better solution.

Sounds good to me! Let's continue the discussion after #60 is finished! 👍🏻

alaister · 2022-06-29T07:08:56Z

Hey @mirceanton,
As in #63 would you mind fixing the merge conflicts?

Also as this function can't ever error I think you're right and we can just change it to:

const { publicURL } = storage.from('my-bucket').getPublicUrl('profile.png')

Sorry that this took so long and thanks again!

mircea-pavel-anton · 2022-06-29T11:55:49Z

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

mircea-pavel-anton · 2022-07-03T09:18:06Z

Hey, @alaister! I solved the merge conflicts, it should be ok now.

One more question though, should we leave the error field in the return type of the method? It's always going to be null so I guess the only argument for it is consistency with the others.

alaister · 2022-07-04T06:41:51Z

Yeah, let's drop the error field. Originally I wanted to keep it to maintain backwards compatibility, but we're releasing next as a breaking change anyway so let's just make it return the publicUrl directly. For example:

const publicUrl = storage.from('my-bucket').getPublicUrl('profile.png')

mircea-pavel-anton · 2022-07-04T10:31:09Z

Done! I also removed the getPublicUrlAsString method since it was now redundant, as getPublicUrl returns the url as a string already.

alaister

Excellent, thank you! The last thing is to update the tests to reflect the new API, if you don't mind?

mircea-pavel-anton · 2022-07-04T11:22:16Z

Yep, already working on it!

mircea-pavel-anton · 2022-07-04T11:31:27Z

That should do the trick!

alaister

Thanks again for all your work @mirceanton!

mircea-pavel-anton · 2022-07-04T11:33:54Z

My pleasure! Let me know if there are any more PRs I can take a look at/help with!

kiwicopple · 2022-08-04T03:19:40Z

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

The release is available on:

Your semantic-release bot 📦🚀

kiwicopple · 2022-08-16T13:23:12Z

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

The release is available on:

Your semantic-release bot 📦🚀

kiwicopple · 2022-10-11T10:49:19Z

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

The release is available on:

Your semantic-release bot 📦🚀

* 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 <[email protected]> Co-authored-by: Bobbie Soedirgo <[email protected]> Co-authored-by: Mircea-Pavel Anton <[email protected]> * 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 <[email protected]> * 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 <[email protected]> Co-authored-by: Bobbie Soedirgo <[email protected]> * 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 <[email protected]> Co-authored-by: Bobbie Soedirgo <[email protected]> * 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 <[email protected]> Co-authored-by: Bobbie Soedirgo <[email protected]> Co-authored-by: Copple <[email protected]> Co-authored-by: Mircea-Pavel Anton <[email protected]> Co-authored-by: Jonathan Picques <[email protected]> Co-authored-by: Bobbie Soedirgo <[email protected]> Co-authored-by: Mircea-Pavel Anton <[email protected]> Co-authored-by: Inian <[email protected]> Co-authored-by: Fabrizio <[email protected]> Co-authored-by: Copple <[email protected]>

mircea-pavel-anton mentioned this pull request Apr 8, 2022

feat: improve function return types #60

Merged

alaister reviewed Apr 8, 2022

View reviewed changes

JonathanPicques and others added 3 commits June 10, 2022 23:41

fix: typo in updateBucket jsdoc

dc7accc

fix: import cross-fetch conditionally

1dbd78e

fix: es2020

4dff623

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

Mircea-Pavel Anton and others added 4 commits July 3, 2022 08:33

Improve getPublicUrl

20bf4da

Make return types consistent across all functions.

165d874

Undo return type change

04e9493

Merge branch 'next' into fix/publicurl

f5d0d2a

mircea-pavel-anton added 2 commits July 4, 2022 10:29

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

8d6661e

Remove redundant method.

8dbdab1

alaister requested changes Jul 4, 2022

View reviewed changes

Update tests.

792c9fd

alaister approved these changes Jul 4, 2022

View reviewed changes

alaister merged commit 0d34f01 into supabase:next Jul 4, 2022

mircea-pavel-anton deleted the fix/publicurl branch July 4, 2022 11:33

kiwicopple added the released on @next label Aug 4, 2022

kiwicopple added the released on @rc label Aug 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve `getPublicUrl` #62

Improve `getPublicUrl` #62

mircea-pavel-anton commented Apr 8, 2022

mircea-pavel-anton commented Apr 8, 2022

alaister left a comment

alaister Apr 8, 2022

alaister Apr 8, 2022

mircea-pavel-anton Apr 8, 2022

alaister Apr 8, 2022

alaister Apr 8, 2022

mircea-pavel-anton Apr 8, 2022

mircea-pavel-anton Apr 8, 2022

mircea-pavel-anton Apr 8, 2022

alaister Apr 8, 2022

mircea-pavel-anton Apr 8, 2022

alaister commented Jun 29, 2022

mircea-pavel-anton commented Jun 29, 2022

mircea-pavel-anton commented Jul 3, 2022

alaister commented Jul 4, 2022

mircea-pavel-anton commented Jul 4, 2022

alaister left a comment

mircea-pavel-anton commented Jul 4, 2022

mircea-pavel-anton commented Jul 4, 2022

alaister left a comment

mircea-pavel-anton commented Jul 4, 2022

kiwicopple commented Aug 4, 2022

kiwicopple commented Aug 16, 2022

kiwicopple commented Oct 11, 2022

Improve getPublicUrl #62

Improve getPublicUrl #62

Conversation

mircea-pavel-anton commented Apr 8, 2022

What is the current behavior?

What is the new behavior?

Additional context

mircea-pavel-anton commented Apr 8, 2022

alaister left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alaister commented Jun 29, 2022

mircea-pavel-anton commented Jun 29, 2022

mircea-pavel-anton commented Jul 3, 2022

alaister commented Jul 4, 2022

mircea-pavel-anton commented Jul 4, 2022

alaister left a comment

Choose a reason for hiding this comment

mircea-pavel-anton commented Jul 4, 2022

mircea-pavel-anton commented Jul 4, 2022

alaister left a comment

Choose a reason for hiding this comment

mircea-pavel-anton commented Jul 4, 2022

kiwicopple commented Aug 4, 2022

kiwicopple commented Aug 16, 2022

kiwicopple commented Oct 11, 2022

Improve `getPublicUrl` #62

Improve `getPublicUrl` #62