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

TypeError: Harvest is not a constructor #108

Closed
sdelpuerto-intelygenz opened this issue Jul 23, 2018 · 23 comments
Closed

TypeError: Harvest is not a constructor #108

sdelpuerto-intelygenz opened this issue Jul 23, 2018 · 23 comments

Comments

@sdelpuerto-intelygenz
Copy link
Contributor

Node version: 8.9.3 / 10.7.0
Harvest version: 2.0.3

const Harvest = require('harvest')
const harvest = new Harvest({/* ... */})

// TypeError: Harvest is not a constructor

What's going on?

@simplyspoke simplyspoke self-assigned this Jul 23, 2018
@simplyspoke
Copy link
Owner

Good question,

I am looking into the issue presently. Initial investigation would suggest a issue relating to recent automated updates. Will rebuild, test, and let you know.

@simplyspoke
Copy link
Owner

I have entirely shifted to import in most of my uses, so looks like I didn't properly test require.

For the immediate moment, const Harvest = require('harvest').default should work. I will update the module loader shortly to support require.

Thank you for reporting this issue.

@sdelpuerto-intelygenz
Copy link
Contributor Author

I figured it would have something to do with imports, but in the meantime,require('harvest').default works indeed.

Thanks for the quick response.

@simplyspoke
Copy link
Owner

Sure thing,

I realized in my foolishness that when I updated the module to support the v2 API, I had bundled it for the es6 import and had not been properly testing require. Once I get some time in the next day or so, I will bundle up a commonjs version and update the package.json to support both.

Would you be willing to verify it once I do?

@sdelpuerto-intelygenz
Copy link
Contributor Author

Yes, of course. Just hit me up when you publish and I'll gladly do the test.

simplyspoke added a commit that referenced this issue Jul 25, 2018
The initial conversion of the module to typescript was bundling a module that supported the ES6
import, but not the commonjs require. Added an index.js to the root and modified the package json.

re #108
@simplyspoke
Copy link
Owner

@sdelpuerto-intelygenz,

Version 2.1.1 should have what you need. Would have been patch version less, but in my thickness I pushed up the fix without including the new file in the publish.

Let me know if there is anything else.

@sdelpuerto-intelygenz
Copy link
Contributor Author

It works now without having to use require('harvest').default (in fact that gives the same error now).

Thank you for the quick fix.

@Jameskmonger
Copy link
Contributor

Jameskmonger commented Aug 30, 2018

This is still broken when using TypeScript.

The typings for "harvest" mean I have to import it as a default:

import Harvest from "harvest";

const harvest = new Harvest(...)

This then transpiles to:

var harvest_1 = require("harvest");
var harvest = new harvest_1["default"]({

However harvest_1 is the Harvest constructor itself. (thus ["default"] is undefined.)

To fix this, the /index.js file needs to be deleted and main in package.json needs to be updated to point to /dist/index.js.

@Jameskmonger
Copy link
Contributor

A workaround for now is to do the following:

const Harvest = require("harvest");

const harvest = new Harvest({

Downsides of this:

  1. You must install @types/node
  2. You don't get any typings for Harvest.

I have raised a fix in #112

simplyspoke added a commit that referenced this issue Sep 1, 2018
Fix #108: Export default class correctly
@simplyspoke
Copy link
Owner

🎉 This issue has been resolved in version 2.2.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@robinparisi
Copy link

I get the same error with version 2.2.3. Maybe a regression?

@simplyspoke simplyspoke reopened this Sep 5, 2018
@simplyspoke
Copy link
Owner

I suspect as much. Will have to get some better testing in place and ensure proper support of both commonjs and ES6 imports.

@Jameskmonger
Copy link
Contributor

I will investigate this shortly @simplyspoke

@Jameskmonger
Copy link
Contributor

I'm not able to reproduce this @robinparisi. Here is my setup:

import Harvest from "harvest";

const api = new Harvest({
    subdomain: 'mysubdomain',
    userAgent: 'myuseragent',
    concurrency: 1,
    auth: { accessToken, accountId } 
});

api.projectAssignments.me({})
    .then(assignments => console.log(assignments));

I get some assignments printed in the console. Would you mind sharing your setup?

@simplyspoke
Copy link
Owner

I believe those experiancing the issue are trying to us the module via commonjs, so it would be using require('harvest');

@Jameskmonger
Copy link
Contributor

@robinparisi A workaround for now is this:

const Harvest = require("harvest").default;

I think some further investigation is needed to see how we can have an export which can be:

  • imported in TypeScript like import Harvest from "harvest";
  • imported in JavaScript like const Harvest = require("harvest");

There is a related issue on the TypeScript repo: microsoft/TypeScript#2719

Anders Hejlsberg's response is a wontfix - either ES6 exports or CommonJS, but not both

@robinparisi
Copy link

@Jameskmonger as @simplyspoke said, I'm using commonjs. No problem with the require("harvest").default, maybe it should be added to the documentation?

@simplyspoke
Copy link
Owner

I am looking at a solution that will support both, my tests suggest the following:

Adjusted package.json:

  "main": "./dist/index.cjs.js",
  "jsnext:main": "./dist/index.js",
  "module": "./dist/index.js",

The contents of index.cjs.js being:
module.exports = require('./').default;

Was also considering a move away from webpack. Doesn't feel like the right tool for the job in this case.

@Jameskmonger
Copy link
Contributor

@simplyspoke I don't think there's a need for webpack (or any bundler) at all. Just compile the typescript into the dist directory and publish that.

Does your solution still work nicely with TypeScript imports?

@simplyspoke
Copy link
Owner

I agree @Jameskmonger...

This is a rather lightweight and simple module, so throwing a bundler in front of the packaging process doesn't feel like it gets us much.

As for the imports, I tested both a ts file and js file just shifting the module loader and it looks like it should fix both needs well. Would like to get a better eye on it before pushing up the changes as I only had 15min to test my approach. Might be nice to pair with pulling out webpack.

@Jameskmonger
Copy link
Contributor

I do notice here that jsnext:main has been superseded by the module property anyway. Do you still think it's worth having both? Maybe for people on older versions.

@simplyspoke
Copy link
Owner

I like having that bit of backwards compatibility. If it reduces the chance people with have any issues, seems like a win.

@simplyspoke
Copy link
Owner

@robinparisi I believe that 2.2.4 should support require now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants