Skip to content
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

fix: detect cids in query parameters #31

Merged
merged 6 commits into from
Jan 19, 2024
Merged

fix: detect cids in query parameters #31

merged 6 commits into from
Jan 19, 2024

Conversation

guanzo
Copy link
Collaborator

@guanzo guanzo commented Jan 18, 2024

Handles #30

CID + path detection is now 1 function instead of 2, and works in these cases:

  • subdomain cid: bafybeidrf56yzbkocajbloyafrebrdzsam3uj35sce2fdyo4elb6zzoily.ipfs.dweb.link/cat.png
  • path cid: https://ipfs.io/ipfs/bafybeidrf56yzbkocajbloyafrebrdzsam3uj35sce2fdyo4elb6zzoily/cat.png
  • query param
    • path cid
      https://proxy.com/?url=ipfs.io%2Fipfs%2Fbafybeidrf56yzbkocajbloyafrebrdzsam3uj35sce2fdyo4elb6zzoily%2Fcat.png
    • subdomain cid
      https://proxy.com/?url=bafybeidrf56yzbkocajbloyafrebrdzsam3uj35sce2fdyo4elb6zzoily.ipfs.dweb.link%2Fcat.png

@guanzo guanzo requested a review from AmeanAsad January 18, 2024 21:33
Copy link
Contributor

@AmeanAsad AmeanAsad left a comment

Choose a reason for hiding this comment

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

LGTM. Some non blocking comments:

  • The findCIDPathInURL and findCIDPathInURLComponent return empty strings when a CID is not found. Wondering if it is better to throw errors in these scenarios as it forces who ever is using those functions to explicitly handle that case. Also makes it easier to understand the expected behavior of the function
  • The findCIDPathInUrlComponent function does a lot of string manipulation. I think some comments would help understand what is happening in the function. In the case we need to add support for more URL format, would also make it easier to understand the code and make necessary changes.

@guanzo
Copy link
Collaborator Author

guanzo commented Jan 19, 2024

Wondering if it is better to throw errors in these scenarios as it forces who ever is using those functions to explicitly handle that case.

I think it's better to return null. It's expected that functions that find/search stuff may not find anything, e.g. Array.find() returns undefined instead of throwing.

I think some comments would help understand what is happening in the function

good point i'll add some comments

@guanzo guanzo merged commit b294f39 into main Jan 19, 2024
1 check passed
@guanzo guanzo deleted the cid-detection branch January 19, 2024 19:34
@gruns
Copy link

gruns commented Jan 22, 2024

I think it's better to return null. It's expected that functions that find/search stuff may not find anything, e.g. Array.find() returns undefined instead of throwing.

agree with eric here. if the function was extractCid() and the expectation was to always find a cid, i think throwing an error would be appropriate. but its expected that most urls wont have cids in them

prior art for context: regex libs dont throw an error when you search a string for a match and none is found. they just return no matches, eg an empty list 🙂

and agree with amean that comments with examples helpful! 🙌

my 2c

either way, awesome PR guys 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants