Skip to content

fix(cloudimage): skip cdn when src path is provided with protocol - #1028

Merged
atinux merged 3 commits into
nuxt:mainfrom
joffreyBerrier:1026/if-src-startwith-http-return-src-for-cloudimage
Oct 4, 2023
Merged

fix(cloudimage): skip cdn when src path is provided with protocol#1028
atinux merged 3 commits into
nuxt:mainfrom
joffreyBerrier:1026/if-src-startwith-http-return-src-for-cloudimage

Conversation

@joffreyBerrier

@joffreyBerrier joffreyBerrier commented Oct 3, 2023

Copy link
Copy Markdown
Contributor

Problem

  • Cloudimage: Images starting with HTTP should not be prefixed with Base Url

issues

Solution

  • Add condition if src startwith http return src with generate query parameters

Before:

  • https://token.cloudimg.io/__path__/https://2412819702-files.gitbook.io/~/files/v0/b/gitbook-x-prod.appspot.com/o/spaces%2FlIgyYELwJG6odLEyCM6i%2Fuploads%2FAHcbuKRYbIlBWO4cJ88b%2Fimage.png?alt=media&token=62ff753d-83eb-4e3f-932c-96eb72d455f1?height=100&width=100&q=65&func=crop

After:

  • https://2412819702-files.gitbook.io/~/files/v0/b/gitbook-x-prod.appspot.com/o/spaces%2FlIgyYELwJG6odLEyCM6i%2Fuploads%2FAHcbuKRYbIlBWO4cJ88b%2Fimage.png?alt=media&token=62ff753d-83eb-4e3f-932c-96eb72d455f1?height=100&width=100&q=65&func=crop

Remaining

  • I have tried to do that for all providers, but the provider manages the logic of the URL, so I can't test this and can't do that for all providers. Do you have any idea?

Comment thread src/runtime/providers/cloudimage.ts Outdated
Comment thread src/runtime/providers/cloudimage.ts Outdated

@danielroe danielroe 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.

Thank you ❤️

I don't think we need to update other providers in this PR.

Could you update the check on src to make it a bit more precise? 🙏

@danielroe danielroe changed the title chore(cloudimage): add condition if url startwith http for cloudimage fix(cloudimage): skip cdn when absolute src path is provided Oct 3, 2023
@danielroe danielroe changed the title fix(cloudimage): skip cdn when absolute src path is provided fix(cloudimage): skip cdn when src path is provided with protocol Oct 3, 2023

@danielroe danielroe 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.

this looks good to me, thank you 👌

danielroe

This comment was marked as duplicate.

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (cb18ed1) 89.69% compared to head (2e49231) 89.57%.
Report is 7 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1028      +/-   ##
==========================================
- Coverage   89.69%   89.57%   -0.12%     
==========================================
  Files          44       44              
  Lines        2920     2927       +7     
  Branches      328      329       +1     
==========================================
+ Hits         2619     2622       +3     
- Misses        300      304       +4     
  Partials        1        1              
Files Coverage Δ
src/runtime/providers/cloudimage.ts 67.64% <50.00%> (-2.85%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@atinux
atinux merged commit 3101019 into nuxt:main Oct 4, 2023
riddla pushed a commit to tricks-gmbh/nuxt-image that referenced this pull request Mar 1, 2024
…uxt#1028)

* chore(cloudimage): add condition if url startwith http for cloudimage

* chore(cloudimage): feedback Daniel

* test: update snapshots for cloudimage

---------

Co-authored-by: Daniel Roe <daniel@roe.dev>
@github-actions github-actions Bot mentioned this pull request Jul 30, 2026
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.

4 participants