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: try parse server.origin URL #19241

Merged
merged 1 commit into from
Jan 20, 2025
Merged

fix: try parse server.origin URL #19241

merged 1 commit into from
Jan 20, 2025

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Jan 20, 2025

Description

fix #19239

This line is causing the latest version to fail: https://github.com/laravel/vite-plugin/blob/e57a940c22f90e72002380d3dad1a2c6f1921983/src/index.ts#L154

I don't think we really expect frameworks to configure the option this way, but they have an interesting usecase that indeed make it seem like we could improve the server.origin option a bit, but maybe later in the future.

Copy link
Member

@patak-dev patak-dev left a comment

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 should keep this fix, but we can do a quick patch with it to avoid disruption in the patch. As you said, this doesn't open up a security problem and it will still work for laravel. We should review this for 6.1 or 7.0

@patak-dev patak-dev merged commit 2495022 into main Jan 20, 2025
15 checks passed
@patak-dev patak-dev deleted the server-origin-try-parse branch January 20, 2025 19:30
try {
const serverOriginUrl = new URL(resolvedServerOptions.origin)
list.push(serverOriginUrl.hostname)
} catch {}
Copy link
Contributor

Choose a reason for hiding this comment

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

should this have a debug log at least? quiet swallowing can lead to long and frustrating debug sessions

Copy link
Member

Choose a reason for hiding this comment

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

we can add one, yes, but I don't expect that this fix will stay in place

@timacdonald
Copy link
Contributor

We really appreciate the Vite team making this consideration for the Laravel community.

We have a PR up to make our placeholder URL a well-formed URL, which should mean this check can be removed and our plugin will continue to work.

laravel/vite-plugin#317

Thank you again for supporting our community!

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.

Vite >6.0.9 has CORS issue with laravel-vite
4 participants