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 url concatenation for dev mode with origin set in server options #732

Merged
merged 2 commits into from
Jul 10, 2024

Conversation

dknibbe
Copy link
Contributor

@dknibbe dknibbe commented Jul 7, 2024

  • Quick Checklist
  • I have read the contributing guidelines
  • I have written new tests, as applicable (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • I have added a changeset, if applicable
  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
    This change contains a bug fix for the feature introduced in pull request Add vite origin support for image urls in development #690. The server origin option usually contains a protocol scheme identifier followed by a double slash, e.g. "http://localhost:1234". Nodes POSIX path implementation is not meant to join URLs, which means the double slash is converted to a single slash, resulting in an invalid URL, e.g "http:/localhost:1234". Using url-join instead ensures the result will be a valid URL.

  • Other information:
    Added new dependency: url-join, which seams to be the safest option

Copy link

changeset-bot bot commented Jul 7, 2024

🦋 Changeset detected

Latest commit: ead219f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
vite-imagetools Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@@ -1,5 +1,5 @@
import { basename, extname } from 'node:path'
import { join } from 'node:path/posix'
import urlJoin from 'url-join'
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we need to add a dependency for such a simple piece of functionality. Can you just add a method here to do it instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed it to string concatenation. This should work fine, because Vite makes sure that the origin option never ends with a '/'.

@dknibbe dknibbe force-pushed the fix-server-origin branch from 5415271 to f6197ca Compare July 8, 2024 19:08
Copy link

codecov bot commented Jul 10, 2024

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 95.69%. Comparing base (31c14f2) to head (ead219f).
Report is 4 commits behind head on main.

Files Patch % Lines
packages/vite/src/index.ts 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #732      +/-   ##
==========================================
- Coverage   95.69%   95.69%   -0.01%     
==========================================
  Files          33       33              
  Lines        1278     1277       -1     
  Branches      224      224              
==========================================
- Hits         1223     1222       -1     
  Misses         55       55              
Flag Coverage Δ
imagetools-core 95.69% <0.00%> (-0.01%) ⬇️
vite-imagetools 95.69% <0.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@benmccann benmccann merged commit c4a0dc7 into JonasKruckenberg:main Jul 10, 2024
5 of 7 checks passed
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.

2 participants