Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix: check if endpoint exists for dynamic pages when checking for red… #6994

Merged
merged 3 commits into from
Jan 15, 2025

Conversation

VitaliyR
Copy link
Contributor

@VitaliyR VitaliyR commented Jan 10, 2025

…irect

🎉 Thanks for submitting a pull request! 🎉

Summary

Fixes WRFL-2122

Basically we are checking only for static files when trying to determinate if we should do the non forced redirect.

In dynamic SSG, when they either run in dev mode or having dynamic endpoints (not a static files), we are not detecting it and doing the redirect, which is wrong.

I've added a simple checker via fetch url, { method: HEAD } - which checks if the endpoint exists.

Example

Assume next netlify.toml:

[[redirects]]
from = "*"
to = "https://www.netlify.app/:splat"
status = 200
force = false # by default

Run netlify dev.

Make change to some code file.

Open http://localhost:8888. See that change is not applied. See in console Proxying to https://www.netlify.app/.

Thats wrong, it should actually detect that there is an index route in current site and not proxy.


For us to review and ship your PR efficiently, please perform the following steps:

  • Open a bug/issue before writing your code 🧑‍💻. This ensures we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing a typo or something that`s on fire 🔥 (e.g. incident related), you can skip this step.
  • Read the contribution guidelines 📖. This ensures your code follows our style guide and
    passes our tests.
  • Update or add tests (if any source code was changed or added) 🧪
  • Update or add documentation (if features were changed or added) 📝
  • Make sure the status checks below are successful ✅

A picture of a cute animal (not mandatory, but encouraged)

@VitaliyR VitaliyR requested a review from a team as a code owner January 10, 2025 01:07
Copy link

github-actions bot commented Jan 10, 2025

📊 Benchmark results

Comparing with 1ff368f

  • Dependency count: 1,201 (no change)
  • Package size: 316 MB (no change)
  • Number of ts-expect-error directives: 830 (no change)

src/utils/proxy.ts Dismissed Show resolved Hide resolved
proxyResHeaders = {
...proxyResHeaders,
'content-length': String(responseBody.byteLength),
if (!proxyResHeaders['transfer-encoding']) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@smnh you added this, but when there is transfer-encoding, you should not send content-length.
sindresorhus/got#1576

WDYT about this change?

Copy link
Contributor

@smnh smnh Jan 14, 2025

Choose a reason for hiding this comment

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

According to MDN, It looks like it should be ommitted only when Transfer-Encoding is chunked.
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Transfer-Encoding#syntax

And it make sense, because for regular non chunked encodings like gzip and deflate you do want to have content-length.

if (proxyResHeaders['transfer-encoding']?.includes('chunked') {
  ...
}

@VitaliyR VitaliyR merged commit db1d87e into main Jan 15, 2025
49 checks passed
@VitaliyR VitaliyR deleted the vitaliir/WRFL-2122 branch January 15, 2025 13:27
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants