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(cloudimage): skip cdn when src path is provided with protocol #1028

Conversation

joffreyBerrier
Copy link
Contributor

@joffreyBerrier joffreyBerrier commented Oct 3, 2023

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?

Copy link
Member

@danielroe danielroe left a comment

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
Copy link
Member

@danielroe danielroe left a comment

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

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
2 checks passed
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 <[email protected]>
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