Support webpack with node environments (e.g. electron)

[d-fischer]

It seems like webpack resolves node-fetch as its ESM version, which contains the method as the default member rather than the root export. This change should gracefully handle that while still preserving the other exports (Request, Response, Headers).

Tested in electron 8.0.1, with and without webpack 4.41.6, both work now.

I'd recommend to publish this as a patch version (6.1.1).

[hata6502]

I agree this PR, too! :)
WebPack mistakes fetch.default. So this PR is needed.

(node:31443) UnhandledPromiseRejectionWarning: TypeError: fetch is not a function
    at eval (webpack://node_modules/fetch-ponyfill/fetch-node.js?:13:12)

[camhart]

What's it going to take to make this happen?

[qubyte]

Thanks for the PR and your patience. This PR will need a test To prevent a regression before it’s admitted.

[camhart]

I added the following to package.json scripts section:

"pretest:webpack:node": "webpack --mode development --target node tests/fetch-node.spec.js -o build/node-test-bundle.js",
    "test:webpack:node": "mocha build/node-test-bundle.js",

Which results in all tests passing except for the Promise context one... I don't fully understand what's going on there though.

C:\Users\Cam\projects\fetch-ponyfill>npm run test:webpack:node

> [email protected] pretest:webpack:node C:\Users\Cam\projects\fetch-ponyfill
> webpack --mode development --target node tests/fetch-node.spec.js -o build/node-test-bundle.js

Hash: 517be8eddf5940aad657
Version: webpack 4.42.1
Time: 475ms
Built at: 05/29/2020 4:56:55 PM
              Asset     Size  Chunks             Chunk Names
node-test-bundle.js  781 KiB    main  [emitted]  main
Entrypoint main = node-test-bundle.js
[./fetch-node.js] 1.08 KiB {main} [built]
[./node_modules/webpack/buildin/module.js] (webpack)/buildin/module.js 497 bytes {main} [built]
[./tests/fetch-node.spec.js] 7.19 KiB {main} [built]
[assert] external "assert" 42 bytes {main} [built]
[events] external "events" 42 bytes {main} [built]
[fs] external "fs" 42 bytes {main} [optional] [built]
[http] external "http" 42 bytes {main} [built]
[https] external "https" 42 bytes {main} [built]
[path] external "path" 42 bytes {main} [built]
[querystring] external "querystring" 42 bytes {main} [built]
[stream] external "stream" 42 bytes {main} [built]
[timers] external "timers" 42 bytes {main} [built]
[url] external "url" 42 bytes {main} [built]
[util] external "util" 42 bytes {main} [built]
[zlib] external "zlib" 42 bytes {main} [built]
    + 37 hidden modules

> [email protected] test:webpack:node C:\Users\Cam\projects\fetch-ponyfill
> mocha build/node-test-bundle.js



  fetch in Node
    when called without a context
      √ uses the built-in promise implementation
      √ exposes fetch, and Request, Response, and Headers methods
      √ returns a promise which resolves to an instance of Response
      √ makes requests
      √ rejects with an error on a bad request
      √ supports schemaless URIs
      √ supports request instances
      √ supports URL instances
    when called with a context with no Promise field
      √ exposes fetch, and Request, Response, and Headers methods
      √ returns a promise which resolves to an instance of Response
      √ uses the built-in promise implementation
      √ makes requests
      √ rejects with an error on a bad request
      √ supports schemaless URIs
      √ supports request instances
      √ supports URL instances
    when called with a context with a Promise field
      √ exposes fetch, and Request, Response, and Headers methods
      √ returns a promise which resolves to an instance of Response
      1) uses the a given promise implementation
      √ makes requests
      √ rejects with an error on a bad request
      √ supports schemaless URIs
      √ supports request instances
      √ supports URL instances


  23 passing (54ms)
  1 failing

  1) fetch in Node
       when called with a context with a Promise field
         uses the a given promise implementation:

      AssertionError [ERR_ASSERTION]: false == true
      + expected - actual

      -false
      +true

      at Context.eval (webpack:///./tests/fetch-node.spec.js?:197:14)
      at processImmediate (internal/timers.js:456:21)



npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! [email protected] test:webpack:node: `mocha build/node-test-bundle.js`
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the [email protected] test:webpack:node script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     C:\Users\Cam\AppData\Roaming\npm-cache\_logs\2020-05-29T23_56_55_817Z-debug.log

[camhart]

var ThenPromise = require('Promise')
...
assert.ok(fetch.fetch('https://mattandre.ws/succeed.txt') instanceof ThenPromise); //this fails
assert.ok(fetch.fetch('https://mattandre.ws/succeed.txt') instanceof Promise); //this succeeds

[camhart]

See #250 for included testing. Or I'm okay with those changes being brought over to this PR. I'm a bit of a github forking/PR novice so sorry about that.

[d-fischer]

The whole point of ThenPromise is that the function can take a Promise implementation that's different from the native one. Your change makes a bunch of tests succeed where they should probably fail.

Also I think it's bad style to just take someone else's code and put it in your own PR - even though it's just one little line.

I'm currently working on fixing the code so it actually passes the test.

[d-fischer]

Guess it looks good now 😄

[qubyte]

Looks good! It looks like the files touched have changed mode from 100644 to 100755 (they've become executable). Can you see if you can change those back? Following that I'll merge and publish a version.

[qubyte]

@camhart in the future there are two approaches I can think of which you can use with GitHub to keep everything properly attributed.

One is to clone a repo, add @d-fischer's fork as a remote and check out the branch, append commits, push to your fork, and then PR the branch on your fork. Authorship of commits should be retained, and if your PR gets merged the earlier PR is automatically closed. This is a method which works if the author of the original PR is perhaps slow to respond, which is not the case here.

The second approach is arguably better since it engages the original author. You make a pull request to the branch on their fork with the commits you want added. If/when that PR is merged this PR would automatically be updated with the new commits and tests re-run.

[d-fischer]

Ah yes, good old WSL and its executable flags everywhere. Fixed.

[qubyte]

Nice work with this, and thanks again for your patience.

[qubyte]

Published as v6.1.1.