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

Add a missing method when checking if an object is a spec-compliant FormData object #876

Closed
wants to merge 1 commit into from

Conversation

xxczaki
Copy link
Member

@xxczaki xxczaki commented Jun 11, 2020

@jimmywarting
Copy link
Collaborator

jimmywarting commented Jun 11, 2020

OCD kicking in? :P
shouldn't we put them in alphabetic order while we are at it? :P

@tinovyatkin
Copy link
Member

tinovyatkin commented Jun 11, 2020

Not exactly related to this PR, but I think we should start this discussion again 😄
I'm pretty skeptical about this way of object testing that we are using in node-fetch. It's completely different things to check an object provided by an environment (DOM for example) just by Symbol.toStringTag vs concluding out of this symbol that some object will act as we believe in an environment where it doesn't belong natively (read FormData in Node.js).
Should we test methods arguments too (set.length)?
I would prefer to have some small reference implementations of Blob, FormData, and AbortController inside the node-fetch organization and just do instanceof - as it can be extended by any developer if needed.
For example, the following class will pass your test perfectly, but will not work as expected:

class FormData extends URLSearchParams {
  get [Symbol.toStringTag]() {
    return "FormData";
  }
}

@jimmywarting
Copy link
Collaborator

jimmywarting commented Jun 11, 2020

I would prefer to have some small reference implementations of Blob, FormData, and AbortController inside the node-fetch organization and just do instanceof

That would increase the size of node-fetch. And you wouldn't be able to use similar polyfills
(hint hint -> #751)

I still prefer to keep none fetch stuff outside of node-fetch. I mean formdata-node has a custom File class that is not inherit by fetch-blob and can't be a instance of the same

@tinovyatkin
Copy link
Member

@jimmywarting we have Blob already, FormData (without serialization) is almost complete in my example above and we have serialization built-in into node-fetch. AbortController is small too.

instanceof can be extended by base class via Symbol.hasInstance as I did today for fetch-blob or can be spoofed via Proxy, as in the example in #848, nothing particularly difficult.

@jimmywarting
Copy link
Collaborator

jimmywarting commented Jun 11, 2020

@tinovyatkin Would like @bitinn or someone else opinion on that first before we decide on it - create a issue about it first - the hasInstance is grate doe :)

@jimmywarting
Copy link
Collaborator

jimmywarting commented Nov 4, 2021

I don't think this is worth merging if #1314 gets merged.
it basically brings in formdata-polyfill and fixes some stuff like the missing body.formData() while also using symbol.hasInstance to check if something is a spec compatible formData

this util function got wiped out completely by
https://github.com/node-fetch/node-fetch/pull/1314/files#diff-598e8a8f4fefeab6b7f348b81147005e33df4922ec007a4d9bb0c2422055bfd8L49

@LinusU
Copy link
Member

LinusU commented Nov 4, 2021

Ah, right, #1314 seems to be very close to getting merged so lets close this one ✔️

@LinusU LinusU closed this Nov 4, 2021
@jimmywarting jimmywarting deleted the missing-form-data branch November 4, 2021 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants