Skip to content

feat: remove methods from passed objects#569

Open
a2937 wants to merge 2 commits intofreeCodeCamp:mainfrom
a2937:feat/remove-methods-from-passed-objects
Open

feat: remove methods from passed objects#569
a2937 wants to merge 2 commits intofreeCodeCamp:mainfrom
a2937:feat/remove-methods-from-passed-objects

Conversation

@a2937
Copy link
Member

@a2937 a2937 commented Feb 27, 2026

Checklist:

Related to freeCodeCamp/freeCodeCamp#66132

While it realistically will not that fix the issue; seeing as how a JSON stringified version of URLSearchParams will return an empty object, this could be pretty neat behavior for other objects being passed around in the future and help them be cloned.

@a2937 a2937 requested a review from a team as a code owner February 27, 2026 22:07
@a2937 a2937 added the blocked This pull-request can not yet be merged label Feb 27, 2026
@ojeytonwilliams
Copy link
Contributor

Hi @a2937 thanks for tackling this, I hadn't even realised there was an issue 😮‍💨

Here's what I think we should do: encode the body exactly how fetch does, set the headers accordingly and post them. E.g. the body new URLSearchParams({ first: 'Mick', last: 'Jagger' }) becomes "first=Mick&last=Jagger" with the content-type set to application/x-www-form-urlencoded;charset=UTF-8.

Does that sound reasonable?

@a2937
Copy link
Member Author

a2937 commented Mar 3, 2026

@a2937
Copy link
Member Author

a2937 commented Mar 3, 2026

@ojeytonwilliams Hold on. This looks like it has the potential to fix this issue too. https://forum.freecodecamp.org/t/advanced-node-and-express-set-up-a-template-engine/780953

@a2937
Copy link
Member Author

a2937 commented Mar 3, 2026

Can we merge this one in then update the body thing in a new PR?

@ojeytonwilliams
Copy link
Contributor

So is this where we're going to be implementing the changes?

https://github.com/freeCodeCamp/curriculum-helpers/blob/80d5b816cb0661f8583e66148c5e7aeb88d3f9c7/packages/shared/src/proxy-fetch.ts

Yes, that's right, I don't awaitable-post should default to modifying the message. At least not preemptively.

Just stringifying by default could cause something else to break and we wouldn't know because we don't test the lessons that interact with a server. For example, fetch can handle circular objects (it just sends "[object Object]"), but JSON.stringify throws when it tries to serialize them.

There's a couple of improvements we can make. One is to implement error handling in awaitable-post (I've been looking into that, so I'd be happy to polish that off and create a PR). The other is to avoid the errors in the first place. fetchProxy should serialize any of the non-cloneable types of body so no errors get thrown in situations we already know how to handle (e.g. URLSearchParams). Is that something you'd be okay with implementing?

@ojeytonwilliams
Copy link
Contributor

Actually, the more I think about it, the less I like the idea of handling DataCloneError. Without re-implementing structuredClone, but falling back to toString and JSON.stringify whenever structuredClone would fail, I don't see how to do it.

Given that, we should focus on avoiding generating the errors. Let me know what you think.

@a2937
Copy link
Member Author

a2937 commented Mar 4, 2026

What kind of tests can I write to make sure createProxyFetch is transforming things correctly. Also note I want to also allow the URL object to be passed as it is used a lot in the Advanced Node and Express section.

https://github.com/freeCodeCamp/freeCodeCamp/blob/0aff3790cd51ef4834a7d3593dd4f4b99be7cf3d/curriculum/challenges/english/blocks/advanced-node-and-express/5895f700f9fc0f352b528e63.md

@ojeytonwilliams
Copy link
Contributor

Good question. To test it, I think it's enough to unit test the function that makes the options both cloneable and ensures they have the right headers. For the first part, they should check that the output of function can be structuredCloned without throwing any errors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

blocked This pull-request can not yet be merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants