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: remove value and writable properties from headers descriptor #12552

Merged
merged 3 commits into from
Dec 2, 2024

Conversation

avanderbergh
Copy link
Contributor

Fixes #12548

  • Ensure that any existing value and writable properties are deleted from the headers descriptor before adding getters and setters.
  • This prevents the error: Invalid property descriptor. Cannot both specify accessors and a value or writable attribute, #<Object>

Changes

  • Removed the value and writable properties from the headers descriptor before defining getters and setters for the headers property on the Request object.
  • Updated the Object.defineProperty call to prevent the Invalid property descriptor error during prerendering.
  • Added a setter to request.headers to allow updating _headers if needed.

Testing

This change was tested by building the Astro project with prerender = true and confirming that the build completes without the Invalid property descriptor error.

No automated tests have been added since this affects the internal build handling of the Request headers property, but manual testing confirms the fix.

Docs

No documentation changes are needed for this fix, as this is an internal implementation detail that does not affect user-facing API or behavior directly.

/cc @withastro/maintainers-docs for feedback if required.

Copy link

changeset-bot bot commented Nov 28, 2024

🦋 Changeset detected

Latest commit: 82a13d0

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

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

@github-actions github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Nov 28, 2024
Copy link

codspeed-hq bot commented Nov 28, 2024

CodSpeed Performance Report

Merging #12552 will degrade performances by 35.68%

Comparing avanderbergh:issue12548 (82a13d0) with main (97f413f)

Summary

⚡ 1 improvements
❌ 1 regressions
✅ 4 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main avanderbergh:issue12548 Change
Rendering: streaming [false], .astro file 1.7 s 1.5 s +15.47%
Rendering: streaming [false], .md file 14.5 ms 22.5 ms -35.68%

@avanderbergh avanderbergh requested a review from bluwy November 28, 2024 14:38
Copy link
Member

@bluwy bluwy 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 fixing this!

- Ensure that any existing value and writable properties are deleted from the headers descriptor before adding getters and setters.
- This prevents error: `Invalid property descriptor. Cannot both specify accessors and a value or writable attribute, #<Object>`
@avanderbergh
Copy link
Contributor Author

@bluwy Hey, I just fixed a merge conflict by rebasing on main, hope that's the right way...

Does it need a new approval now? 😅

@avanderbergh avanderbergh requested a review from bluwy December 2, 2024 08:19
@bluwy
Copy link
Member

bluwy commented Dec 2, 2024

No problem. LGTM!

@bluwy bluwy merged commit 15f000c into withastro:main Dec 2, 2024
15 of 16 checks passed
@astrobot-houston astrobot-houston mentioned this pull request Dec 2, 2024
@avanderbergh
Copy link
Contributor Author

Thank you! 🙌🏻

@avanderbergh avanderbergh deleted the issue12548 branch December 2, 2024 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope)
Projects
None yet
2 participants