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

UnhandledPromiseRejectionWarning: TypeError: Right-hand side of 'instanceof' is not an object #13

Closed
MichaelLeeHobbs opened this issue Sep 18, 2020 · 7 comments

Comments

@MichaelLeeHobbs
Copy link

With "node-fetch": "2.6.0" and "node-fetch-cookies": "1.4.2" the issue does not exist. With either "node-fetch": "2.6.1" or "node-fetch-cookies": "1.5.0" the below issue happens.

(node:34956) UnhandledPromiseRejectionWarning: TypeError: Right-hand side of 'instanceof' is not an object
    at fetch (C:\Users\Michael Hobbs\WebstormProjects\exa\exa-event-service\node_modules\node-fetch-cookies\src\index.mjs:40:29)
    at ExaBot._fetch (C:\Users\Michael Hobbs\WebstormProjects\exa\exa-event-service\src\ExaBot.js:89:25)
    at ExaBot._login (C:\Users\Michael Hobbs\WebstormProjects\exa\exa-event-service\src\ExaBot.js:103:30)
    at Object.<anonymous> (C:\Users\Michael Hobbs\WebstormProjects\exa\exa-event-service\src\index.js:17:8)
    at Module._compile (internal/modules/cjs/loader.js:1137:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1157:10)
    at Module.load (internal/modules/cjs/loader.js:985:32)
    at Function.Module._load (internal/modules/cjs/loader.js:878:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:71:12)
    at internal/main/run_main_module.js:17:47

package for reference

{
  "name": "exa-event-service",
  "version": "0.1",
  "dependencies": {
    "esm": "^3.2.25",
    "express": "^4.17.1",
    "ioredis": "^4.17.3",
    "morgan": "^1.10.0",
    "node-fetch": "2.6.0",
    "node-fetch-cookies": "1.4.2",
    "socket.io": "^2.3.0"
  },
  "browserslist": [
    "last 1 Chrome versions"
  ],
  "scripts": {
    "start": "node src/index",
    "test:exaBot": "node src/exaBot.test.js"
  }
}
MichaelLeeHobbs referenced this issue Sep 18, 2020
export more node-fetch exports
use isRedirect function directly from node-fetch
fix #11
release 1.5.0
@jkhsjdhjs
Copy link
Owner

You're using nodejs 11.14 or 11.15, right? I noticed that the CI for 11.14 failed yesterday, but the CI also auto-releases when just one of the three tests succeed. That's a bit stupid, but that's how it went.
The problem is, that node-fetch 2.6.x is CJS and that importing CJS modules doesn't completely work in nodejs 11:

import nodeFetch from "node-fetch";
console.log(nodeFetch);
// { [Function: fetch] isRedirect: [Function], Promise: [Function: Promise] }

As you can see, nodeFetch doesn't provide exports like Headers or Request here, only the exported functions are available. That's why Headers is undefined at instanceof.
At this point I'm not quite sure what I can do to fix this.

@MichaelLeeHobbs
Copy link
Author

In the container using 'FROM node:12.13.0' and on my dev workstation, I have v12.18.3. Normally I don't run the project directly on the dev workstation but did so to troubleshoot this issue. We first saw the issue in production after pushing out a minor update. Once I realize it was an issue in a library and not the changes I had made I did testing on my local workstation to pin down the issue. After locking the libs to "node-fetch": "2.6.0" and "node-fetch-cookies": "1.4.2" by removing the '^' from them and pushing that out to the pipeline/rebuilding/deploying the issue went away in prod. We will be moving to Node 14 once it goes LTS. This project is still in MVP stage so sadly testing did not catch this issue.

@MichaelLeeHobbs
Copy link
Author

I think your import statement is wrong.

node-fetch/lib/index.mjs

export default fetch;
export { Headers, Request, Response, FetchError };

I think you should use

import nodeFetch, {Headers}  from "node-fetch"

See: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/import

I'm by no means an expert or import as been using require for years 😄

@jkhsjdhjs
Copy link
Owner

jkhsjdhjs commented Sep 19, 2020

I think your import statement is wrong.
I think you should use

import nodeFetch, {Headers}  from "node-fetch"

See: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/import

Yes, you would be correct if node-fetch was an ES module. It uses import/export, but it doesn't have .mjs file extensions or "type": "module" in its package.json, so nodejs views it as CommonJS (the classic require()/module.exports system).
Thus, when trying to import nodeFetch, {Headers} from "node-fetch" I get the following error:

import nodeFetch, {Headers} from "node-fetch";
                   ^^^^^^^
SyntaxError: The requested module 'node-fetch' is expected to be of type CommonJS, which does not support named exports. CommonJS modules can be imported by importing the default export.
For example:
import pkg from 'node-fetch';
const {Headers} = pkg;
    at ModuleJob._instantiate (internal/modules/esm/module_job.js:97:21)
    at async ModuleJob.run (internal/modules/esm/module_job.js:136:20)
    at async Loader.import (internal/modules/esm/loader.js:179:24)

I also tested node-fetch-cookies with both, 12.13.0 and 12.18.3 locally. It works fine for me with both versions. I can only reproduce your error with node 11.14.0 or 11.15.0. I really don't know what's going on on your end.
Can you put a console.log(nodeFetch) in the index.mjs of node-fetch-cookies and send me the output?

EDIT: Ok, with node 11.14 or 11.15 I can import nodeFetch, {Headers} from "node-fetch" - these versions probably don't distinguish between CJS and MJS yet. However, it doesn't work with node 12+. The easiest way to resolve this is probably to drop support for node 11 - nobody is probably using it anyways. But I still don't know what causes this issue on your end.

@MichaelLeeHobbs
Copy link
Author

This is what I see in node_modules/node-fetch/package.json

{
  "name": "node-fetch",
  "version": "2.6.0",
  "description": "A light-weight module that brings window.fetch to node.js",
  "main": "lib/index",
  "browser": "./browser.js",
  "module": "lib/index.mjs",
  "files": [
    "lib/index.js",
    "lib/index.mjs",
    "lib/index.es.js",
    "browser.js"
  ],
...

console.log(nodeFetch) with "node-fetch": "^2.6.0" and "node-fetch-cookies": "^1.4.2"

$ node src/index
[Function: fetch] {
  isRedirect: [Function],
  Promise: [Function: Promise]
}

Based on the above discussion I was able to fix the issue by modifying index.mjs in node-fetch-cookies

import nodeFetch, {Headers, Request, Response, FetchError} from "node-fetch";
import CookieJar from "./cookie-jar.mjs";
import Cookie from "./cookie.mjs";
import {paramError, CookieParseError} from "./errors.mjs";

// const {Headers, Request, Response, isRedirect, FetchError} = nodeFetch;
const {isRedirect} = nodeFetch;

package.json

{
  "name": "exa-event-service",
  "version": "0.1",
  "dependencies": {
    "esm": "^3.2.25",
    "express": "^4.17.1",
    "morgan": "^1.10.0",
    "node-fetch": "^2.6.0",
    "node-fetch-cookies": "^1.4.2",
    "socket.io": "^2.3.0"
  },
  "browserslist": [
    "last 1 Chrome versions"
  ],
  "scripts": {
    "start": "node src/index",
    "test:exaBot": "node src/exaBot.test.js"
  }
}

node -v v12.18.3

I also realized I never pointed out I am importing node-fetch-cookies with esmImport

const CONFIG = require('./config.js')
const esmImport = require('esm')(module)
const {CookieJar, fetch} = esmImport('node-fetch-cookies')

@jkhsjdhjs
Copy link
Owner

I also realized I never pointed out I am importing node-fetch-cookies with esmImport

Ah, so that is why we're getting different results. Thanks for the heads up!

An explanation of the problem can be found here: https://stackoverflow.com/a/62319801/4780052
Also related:
standard-things/esm#880
nodejs/node#33795

Couldn't find a satisfying solution that allows both, nodejs and std/esm import, yet.

@jkhsjdhjs
Copy link
Owner

jkhsjdhjs commented Nov 1, 2020

Since 14.5.1 is now LTS, I dropped support for nodejs versions < 14.13 in a new major release 2.0.0, which resolves this issue.
I also changed the travis ci config such that it now only deploys if the tests on all three nodejs versions succeed, namely latest, latest lts and 14.13, the oldest supported version. This way an accidental release like last time shouldn't happen anymore.

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

No branches or pull requests

2 participants