Making this library isomorphic

[char0n]

Hi,

Would you be open to make this library isomorphic by introducing the package.json browser field?

The browser field will point to lib/browser.js file containing following code:

module.exports = typeof self == 'object' ? self.FormData : window.FormData;

This basically allows to use native FormData object from browser env. In any other case, the implementation of FormData object provided in /lib will be used.

If agreed I can issue a PR. The only effect for existing users will be that webpack and other bundlers will not be bundling this library any more (which they shouldn't do in the first place), but rather use browser field to get FormData object.

PS: I've seen people compensating for missing browser field couple of times already like here: https://github.com/tim-lai/isomorphic-form-data.

[octet-stream]

Sure, why not.

[octet-stream]

Please note that formdata-node itself requires Node 12.4 and newer.

[char0n]

@octet-stream thanks for quick answer. Issuing the PR in couple of minutes.

Please note that formdata-node itself requires Node 12.4 and newer.

Would be great to have this information in README. I'll incorporate this info in my PR as well.

[octet-stream]

This is mentioned at engine section of package.json so an attempt to install this library on lower version should be failed by a package manager (at least Yarn will fail, not sure about pnpm and npm). I just warn that it will be a breaking change for the package from linked issue so you can handle this changes as you do in your package.

[octet-stream]

Issuing the PR in couple of minutes.

Sure. Thank you 👍 I will then wait for it so I can include this enhancement in next release (v3.6.0).

[char0n]

This is mentioned at engine section of package.json so an attempt to install this library on lower version should be failed by a package manager

Yeah, I dealt with this myself in the past. Decided to remove the engines field as it cause more problems as it solves. Some reference here: https://github.com/swagger-api/swagger-js/commit/9c60723f748299f27d603684db9ae25c136d8ab0#commitcomment-41361856

To sum it up, npm takes this field as advisory only, but yarn will refuse to install it. That's bad because I can still utilize babel to transpile the library along with my code and it would work even on older node versions. This is just a tough thought...

[char0n]

PR issued https://github.com/octet-stream/form-data/pull/39

[octet-stream]

The new version is out, you can try it now: https://github.com/octet-stream/form-data/releases/tag/v3.6.0

[char0n]

Thanks!

I'll leave this open for reference of: https://github.com/octet-stream/form-data/pull/39#discussion_r676195955

[char0n]

Finished testing. The current solution doesn't work in webpack@5 due to this small intricacy:

exports field is preferred over other package entry fields like main, module, browser or custom ones.

In order to fix this we have to introduce mapping as proposed in my https://github.com/octet-stream/form-data/pull/39#discussion_r676195955. Tested in locally (by direct override in node_modules) and it works like a charm.

Issued a PR to fix it: https://github.com/octet-stream/form-data/pull/40

[octet-stream]

Let's try a new version: https://github.com/octet-stream/form-data/releases/tag/v3.6.1

[octet-stream]

Just noticed that browser field still there https://github.com/octet-stream/form-data/blob/44a8ff946e857b3a242a0bb8adc7352f73f78622/package.json#L19 Should I just remove it or it will be necessary for fallback and its path just need to be replaced with the one to CJS version of this helper?

[char0n]

Yep let’s point to CJS version of browser.js. I will act as legacy for old versions of bundlers that does not recognize ‘exports’ field

[octet-stream]

Done https://github.com/octet-stream/form-data/releases/tag/v3.6.2

[char0n]

Thanks for swift release; tests for webpack integration are currently running here: https://github.com/swagger-api/swagger-js/pull/2154

[octet-stream]

Looks like one of tests failed because of async generators

[octet-stream]

Which is strange because they seem to be available since 10.3 according to https://node.green/

[octet-stream]

Ah, I see, your tests run under Node 8.

[char0n]

The changes we made to formdata-node browser mapping works like a charm.

Ah, I see, your tests run under Node 8.

Yeah, so far our CJS build artifacts worked even on Node 8. By introducing formdata-node CJS artifacts now require 12.4 as you mentioned earlier. But given that we plan to update node-fetch to v4 which is using formdata-node under the hood and requires 12.20.x we should be fine.

[octet-stream]

But given that we plan to update node-fetch to v4

Probably you meant node-fetch v3. Note that they will require ESM. See https://github.com/node-fetch/node-fetch/pull/1141

By the way, you can still use formdata-node with node-fetch v2 if you will use form-data-encoder to perform form-data encoding.

[char0n]

Probably you meant node-fetch v3 and they also will require ESM. See node-fetch/node-fetch#1141

Yeah, I mean v3 of course

By the way, you can still use formdata-node with node-fetch v2 if you will use form-data-encoder to perform form-data encoding.

Hm, we plan to use node-fetch@v2 with formdata-node@3.6.2. We directly pass FormData instance as a Request.body to node-fetch and everything seems to be working fine. Can you elaborate why we'd need form-data-encoder?

[octet-stream]

Because node-fetch v2 does not have support for spec-compatible form-data implementations, they only handle form-data package, which is not spec-compatible and lacks most of its API. See 2.x branch code: https://github.com/node-fetch/node-fetch/blob/b5e2e41b2b50bf2997720d6125accaf0dd68c0ab/package.json#L53 comparing to master: https://github.com/node-fetch/node-fetch/blob/b50fbc105755123cad34fb0bc9d5653ecc693b8a/package.json#L57-L58

Why we'd need form-data-encoder

form-data-encoder will handle form-data encoding for you while you use an HTTP client in Node.js environment which does not handle spec-compatible FormData, so this is quite useful for node-fetch@2. Once you'll upgrade it to v3 you can just drop form-data-encoder.

I think this needed to be mentioned too: https://github.com/node-fetch/node-fetch/issues/1167

[octet-stream]

You can try send a request with formdata-node + node-fetch@v2 against https://httpbin.org/post with some file (or Blob/Buffer) and regular field.

[char0n]

I assumed that they fully support formdata-node in v2. Here is a whole section describing that they do support it: https://github.com/node-fetch/node-fetch/blob/b50fbc105755123cad34fb0bc9d5653ecc693b8a/README.md#post-data-using-a-file-stream

[octet-stream]

This is readme for v3 :)

[octet-stream]

And this is for v2: https://github.com/node-fetch/node-fetch/blob/8c197f8982a238b3c345c64b17bfa92e16b4f7c4/README.md#post-with-form-data-detect-multipart

[char0n]

Right, thanks. So this means we have two options:

1. use form-data-encoder
2. wait for node-fetch@3 which might take some time before it's out of beta

form-data-encoder option might seem like a way to go for now, and it doesn't need to be isomorphic as it will handle native FormData and FormData from this lib.

[octet-stream]

form-data-encoder does not have any Node.js specific dependencies, so I think it can be bundled. But I haven't tested this use-case. If you'll encounter any problem with it - just file an issue there (in form-data-encoder repo) and we will figure out a fix :)

[char0n]

Looks like using node-fetch@3 is going to be simpler in our case then using form-data-encoder with node-fetch@2.

Anyway thanks for deep insight into all these libraries and how they work together.

[octet-stream]

Alright then.

[char0n]

Managed to successfully integrate formdata-node + form-data-encoder + node-fetch@v2 in https://github.com/swagger-api/swagger-js/pull/2154.

You'll probably see significant formdata-node downloads after merged and released ;]

Yeah and thanks again for providing context for me during our conversations!

[octet-stream]

Yeah, saw that last time you've tried to use my library in the past :D Too bad you didn't manage to reach me out that time, we could've probably find a solution for those issues. Hope it will suit you well this time.

[char0n]

My colleague was handling it at that time and there was a time pressure. Now I had free Sunday and responsive library maintainer so it was pleasure ;]