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

feat(plugins): send accept header in webServer url checking #16251

Merged

Conversation

ghost91-
Copy link
Contributor

@ghost91- ghost91- commented Aug 4, 2022

This PR sets the Accept HTTP header to */* for HTTP requests made by the webServer plugin when using a URL to check if the server has already started. So far, the Accept header was not being sent at all, which should be semantically equivalent to sending */*, according to RFC 7231. However, there are some web servers that handle this differently. Most notably, Vite's server just returns a 404 if the Accept header is missing from the request. I created vitejs/vite#9520 in order to address the issue on their side, but I think it's also fine to just add the header here, which is why I created this PR.

@ghost
Copy link

ghost commented Aug 4, 2022

CLA assistant check
All CLA requirements met.

@ghost91-
Copy link
Contributor Author

ghost91- commented Aug 9, 2022

The test failures seem to be completely unrelated, from what I can tell.

Is there anything I can do to help this PR to get a review?

@mxschmitt mxschmitt requested a review from rwoll August 9, 2022 09:04
Copy link
Member

@rwoll rwoll left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Minor change needed, but otherwise LGTM.

The CI failures are unrelated. (If you re-base—although not necessary—they should go away.)

tests/playwright-test/web-server.spec.ts Outdated Show resolved Hide resolved
@ghost91- ghost91- force-pushed the send-accept-header-in-webserver-requests branch from 3e8956f to 51a5b47 Compare August 10, 2022 08:00
@rwoll rwoll self-requested a review August 10, 2022 19:47
@rwoll rwoll merged commit 3e67a7c into microsoft:main Aug 10, 2022
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