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(fetch): Explicitly check for FormData type #1451

Merged
merged 1 commit into from May 19, 2022

Conversation

timdp
Copy link
Contributor

@timdp timdp commented May 18, 2022

On one hand, the current implementation of the core isFormDataLike helper is:

return chunk && chunk.constructor && chunk.constructor.name === 'FormData'

On the other hand, the Fetch code has an actual FormData implementation.

The latter's constructor.name is 'FormData', so the combination of the two does work. So far, so good.

Now, I'm using esbuild to bundle undici into my app. Because of some naming conflict, it renames the class to FormData3. And then, all hell breaks loose.

To avoid that issue, this PR makes the Fetch part explicitly check the type of the given object.

It'd probably be better to update the core helper instead. I'd originally started to do that. Unfortunately, it introduced a circular dependency. It seemed solvable, but I didn't want to start moving things around just yet.

@targos
Copy link
Member

targos commented May 18, 2022

Is it spec-compliant to do an instanceof check? This would remove support for alternative implementations of FormData.

@ronag
Copy link
Member

ronag commented May 18, 2022

Is it spec-compliant to do an instanceof check? This would remove support for alternative implementations of FormData.

It still does the FormDataLike check.

@NMinhNguyen
Copy link
Contributor

What if we did something like static name = 'FormData' in the class definition instead? That would avoid needing an instanceof check which is a common source of problems in cross-realm code.

@KhafraDev
Copy link
Member

Not needed, the current util.isFormDataLike check is still there if the instanceof check fails. Plus, FormData.name already exists because it's a class.

@NMinhNguyen
Copy link
Contributor

Not needed, the current util.isFormDataLike check is still there if the instanceof check fails.

So there can't be a case where a FormData class is both mangled and imported from another realm (or perhaps another version of the same package on npm) and is passed to isFormDataLike?

Plus, FormData.name already exists because it's a class.

That's precisely why defining it as a static property would override its potentially mangled name.

@ronag
Copy link
Member

ronag commented May 19, 2022

I don't think this makes things worse. Might be room for further improvement as per @NMinhNguyen's comments.

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.

None yet

5 participants