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

Uncaught TypeError: Common is not a constructor #978

Closed
jahidshohel opened this issue Dec 2, 2020 · 28 comments
Closed

Uncaught TypeError: Common is not a constructor #978

jahidshohel opened this issue Dec 2, 2020 · 28 comments

Comments

@jahidshohel
Copy link

My Nodejs version is v15.1.0 and my package.json looks like -

  "name": "counter",
  "version": "1.0.0",
  "description": "",
  "main": "src/index.js",
  "directories": {
    "test": "test"
  },
  "scripts": {
    "start": "node src/index.js",
    "test": "echo \"Error: no test specified\" && exit 1"
  },
  "author": "Jahid Shohel",
  "dependencies": {
    "@ethereumjs/common": "^2.0.0",
    "@ethereumjs/tx": "^3.0.0",
    "@truffle/contract": "^4.2.30",
    "@truffle/hdwallet-provider": "^1.2.0",
    "web3": "^1.3.0"
  },
  "type": "module"
}

In my code I do -

import Common from '@ethereumjs/common';
const c = new Common({ chain: 'ropsten' })

Which gives me Uncaught TypeError: Common is not a constructor. I also tried with import { Common } from '@ethereumjs/common'; and import * as Common from '@ethereumjs/common'; but still the same.

Is this a bug? Or I am doing something wrong?

@jochem-brouwer
Copy link
Member

Hi there, I can indeed reproduce this, I am not yet sure if this is a bug or not since we use these imports a lot in the package without any problem. That might be because these run on typescript? Any thoughts here @holgerd77 @ryanio ?

Anyways, you can temporarily fix the problem using this;

import {default as common} from '@ethereumjs/common';
const Common = common.default
const c = new Common({ chain: 'ropsten' })

@ryanio
Copy link
Contributor

ryanio commented Dec 2, 2020

that's weird.

packages/common/src/index.ts defines export default class Common
so it should work with the simple default import import Common from '@ethereumjs/common'

what happens if you try
const Common = require('@ethereumjs/common').default

i'm also not too familiar with node v15 yet and what changes may be affecting this, can you try on node v12?

@jochem-brouwer
Copy link
Member

const Common = require('@ethereumjs/common').default this works on node v12 and node v15, but only if you do not set the package type to module in package.json (cannot use require in module packages apparently).

@jahidshohel
Copy link
Author

@jochem-brouwer Yes with require('@ethereumjs/common').default it works, but then I can't use modules (e.g: imports).

@ryanio Sam with v12

@holgerd77
Copy link
Member

Can also reproduce, adding to the analysis here.

Put the following code into a script test.js and then ran node test.js:

import Common from '@ethereumjs/common'
const c = new Common({ chain: 'ropsten' })

The package.json file is the default from npm init with an aded "type": "module" directive:

{
  "name": "test",
  "version": "1.0.0",
  "description": "",
  "main": "index.js",
  "scripts": {
    "test": "echo \"Error: no test specified\" && exit 1"
  },
  "author": "",
  "license": "ISC",
  "dependencies": {
    "@ethereumjs/common": "^2.0.0"
  },
  "type": "module"
}

Following behavior:

  1. Node v15.1.0: same error (TypeError: Common is not a constructor)
  2. Node v14.1.0: same error
  3. Node v12.15.0: different error (Cannot use import statement outside a module)

Running with ts-node works fine in all setups here.

@TimDaub
Copy link
Contributor

TimDaub commented Feb 28, 2021

I just reported the same problem here.

Node v12.15.0: different error (Cannot use import statement outside a module)

This is because ESM support landed in node v13.2.0. It seems TSC is flakely supporting ESM .mjs output files.

A way to use them is explained here. However, when vm's tsconfig.prod.json is adjusted:

{
  "extends": "@ethereumjs/config-typescript/tsconfig.prod.json",
  "compilerOptions": {
    "outDir": "./dist",
    "lib": ["dom"],
    "resolveJsonModule": true,
    "target": "ES2015",
    "module": "ES2015"
  },
  "include": ["lib/**/*.ts"]
}

A new problem with arise. When now trying to import, node will complain that no file type extensions are used. Node.js file extensions in the import syntax are mandatory:

node index.mjs
internal/process/esm_loader.js:74
    internalBinding('errors').triggerUncaughtException(
                              ^

Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/Users/user/Projects/ethereumjs-monorepo/packages/vm/dist/state/index' imported from /Users/user/
Projects/ethereumjs-monorepo/packages/vm/dist/index.js
    at finalizeResolution (internal/modules/esm/resolve.js:276:11)
    at moduleResolve (internal/modules/esm/resolve.js:699:10)
    at Loader.defaultResolve [as _resolve] (internal/modules/esm/resolve.js:810:11)
    at Loader.resolve (internal/modules/esm/loader.js:86:40)
    at Loader.getModuleJob (internal/modules/esm/loader.js:230:28)
    at ModuleWrap.<anonymous> (internal/modules/esm/module_job.js:56:40)
    at link (internal/modules/esm/module_job.js:55:36) {
  code: 'ERR_MODULE_NOT_FOUND'
}

So can we somehow magically add file extensions to the files of dist? TSC has decided that they don't want to add file extensions for imports. There's good reasons they won't automatically add it. It's because a compiler wouldn't be able to know when to add what extension. For details see this comment "Fact 2: It's not as simple as "add extension"".

@TimDaub
Copy link
Contributor

TimDaub commented May 16, 2021

esm support has landed in TypeStrong/ts-node#1007

@holgerd77
Copy link
Member

Update: this issue is important and should get some more attention.

@ryanio
Copy link
Contributor

ryanio commented Jun 12, 2021

Thanks @TimDaub for sharing.

hmmm, just reviewing the whole issue, so the crux of the problem seems to be that typescript doesn't output .mjs files, so when using "type": "module" in package.json with vanilla javascript it runs into problems finding the files. Is this right?

A suggested solution on the consumer side seems to be that you can reference the .js file directly and then it should work (would this look like e.g. import Common from '@ethereumjs/common/dist/index.js'? or maybe this only works with node -r ts-node/register or --loader ts-node/esm ), but that's not a dev friendly solution, ideally we should get this working out of the box.

If we provide .mjs files in our dist would this resolve the issue? If so can we write some custom code in our build process to do this while we wait for more native ts support?

When I get some time I will try to answer these questions for myself and report back with results.

@TimDaub
Copy link
Contributor

TimDaub commented Jun 12, 2021

hmmm, just reviewing the whole issue, so the crux of the problem seems to be that typescript doesn't output .mjs files, so when using "type": "module" in package.json with vanilla javascript it runs into problems finding the files. Is this right?

You can find the (hopefully) reproducible introductions to the problems here: #1131

A suggested solution on the consumer side seems to be that you can reference the .js file directly and then it should work (would this look like e.g. import Common from '@ethereumjs/common/dist/index.js'

In the past @ethereumjs/common/dist/index.js has been considered an illegal ModuleSpecifier. It is, however, showing up as a legal "Bare Specifier" in node.js's documentation again. I'm not sure what is the latest agreement here.

Anyways, I'm not aware that JavaScript had a standardized approach for opening an organization scope e.g. @ethereumjs in the ModuleSpecifier. I think that's something informal some projects use, but I'm not sure (nodejs subpath exports). Anyways, if I can do smth like: import Common from '<some ethereumjs common module specifier name>' (without the default as Common), I'm happy.

If we provide .mjs files in our dist would this resolve the issue?

I don't know, we can of course try it.

If so can we write some custom code in our build process to do this while we wait for more native ts support?

I'm not sure if it is possible to write trivial code that transforms require code into esm (e.g. import ... from ...) code. I have tried solving that on my end already and I failed. There are many potential pitfalls.

In Feb I posted that TSC is not supporting .mjs output. The issue seems to be still open. An internet search also didn't yield anything useful. I'm not sure what to do. Anyways here's a picture of Bill 😆

image

@holgerd77
Copy link
Member

holgerd77 commented Jun 16, 2021

This is also not "the solution" [tm], but I guess it would ease things (mid-term) if we - as we are already planning for the VM - remove the Common default export and replace with a named one, so that import is with:

import { Common } from '@ethereumjs/common'

Have added this to the v6 (so: all breaking releases) planning notes.

Let me know if this assumption is false though.

Update: respectively I assume we can directly do this in parallel respectively export both like we have done (or: have not done yet, still not merged) on the wallet library here: ethereumjs/ethereumjs-wallet#145

Can someone please confirm that this would improve things? 😄

@holgerd77
Copy link
Member

(changes to the comment, please read on-site and not in your mail client)

@holgerd77
Copy link
Member

Additional note: I've done a search for "export default class" throughout the monorepo and this actually reveals a lot of occurrences (26 results). So this is somewhat of a bigger decision and effort (manageable though) and should be handled and decided with some minimal care.

Also: is "export default function" also a problem?

@holgerd77
Copy link
Member

Did a test on this and this impressively did not solve the issue. So I adopted the Common class, following diff:

image

Then I created a file test.mjs which I ran with node with node test.mjs:

import { Common } from '@ethereumjs/common'
const c = new Common({ chain: 'mainnet' })

This resulted in:

import { Common } from '@ethereumjs/common'
         ^^^^^^
SyntaxError: Named export 'Common' not found. The requested module '@ethereumjs/common' is a CommonJS module, which may not support all module.exports as named exports.
CommonJS modules can always be imported via the default export, for example using:

import pkg from '@ethereumjs/common';
const { Common } = pkg;

I then tested this solution proposed with the original unedited Common version but this comes back to:

const common = new Common({ chain: 'baikal', hardfork: 'london' })
               ^

TypeError: Common is not a constructor

Phew. 🙁

@holgerd77
Copy link
Member

Haven't digged deeper, but here is a proposed solution to create hybrid modules supporting ESM and CommonJS with TypeScript as a source: https://www.sensedeep.com/blog/posts/2021/how-to-create-single-source-npm-module.html

Would need some analysis what this means for our codebase and distribution process and what might be potential pitfalls or drawbacks though.

@holgerd77
Copy link
Member

Update: I am just through the article, still sounds interesting too me, nevertheless some substantial change to our build process if we decide to do.

One can test such a module in practice by doing npm i dynamodb-onetable (that's one of the packages the author applied this solution to) in a test directory and then examine the installation content, e.g. ll node_modules/dynamodb-onetable/dist/mjs/.

@TimDaub
Copy link
Contributor

TimDaub commented Jun 16, 2021

So I adopted the Common class, following diff:

Hey, I think you'll have to pick between either export default Common or export { Common }. I don't think you can have both in a single file.

Anyways here's the import results they should yield

export default class Common {}
// should allow you to use the following import statement in another file
import Common from "@ethereumjs/common";
export class Common {}
// should allow you to use the following import statement in another file
import { Common } from "@ethereumjs/common";
class Common {}
export { Common };
// should allow you to use the following import statement in another file
import { Common } from "@ethereumjs/common";

@TimDaub
Copy link
Contributor

TimDaub commented Jun 16, 2021

SyntaxError: Named export 'Common' not found. The requested module '@ethereumjs/common' is a CommonJS module, which may not support all module.exports as named exports.
CommonJS modules can always be imported via the default export, for example using:

I believe this is the problem I have with Typescript. That its output is an ES5 build with CommonJS modules (probably using require). It's why I said: If Typescript would start supporting ESM syntax in their output, the problem could be easily resolved by adjusting your Typescript configuration. It's also why I'm slightly criticizing Microsoft, as IMO they should have long committed to shipping ESM. All I could find, however, is some flaky comments from contributors. Excuse my ignorance if there's an official statement. I may not be aware.

Haven't digged deeper, but here is a proposed solution to create hybrid modules supporting ESM and CommonJS with TypeScript as a source: https://www.sensedeep.com/blog/posts/2021/how-to-create-single-source-npm-module.html

Looks interesting. I'll also give it a read once I have time.

@TimDaub
Copy link
Contributor

TimDaub commented Jun 16, 2021

Also: is "export default function" also a problem?

From my side, no. I'm fine with using a library that exports a function. It's equivalent to the following commonJS syntax:

module.exports = function add(a, b) { return a+b };
// is equivalent to ESM
function add(a, b) { return a+b };
export default add;

@karankurbur
Copy link

Running into this issue as well

@Meshugah
Copy link

Meshugah commented Aug 5, 2021

Everyone is

@ARJUN-R34
Copy link

Running into this issue as well

I also faced a similar issue. After some hardcore searches and debugging, I got a fix for this. Let me know if this works.
const Common = require('@ethereumjs/common').default

@TimDaub
Copy link
Contributor

TimDaub commented Sep 13, 2021

FYI, maybe the issue can be resolved by following this guide: https://2ality.com/2019/10/hybrid-npm-packages.html

@holgerd77
Copy link
Member

Just to add that we have also an issue open on this (opened couple of days ago): #1468

@TimDaub thanks, I am also taking this one since it's a bit newer and might be a bit more up-to-date (?) on the issue.

Would it work if we do an additional ESM build to dist.esm along with our current two builds dist (ES2017 target) and dist.browser (ES5 target) and then do additions to the package.json file taking inspirations from the article I mentioned like this:

"files": [
  "dist",
  "dist.browser",
  "dist.esm",
  "src"
],
"main": "dist/index.js",
"types": "dist/index.d.ts",
"browser": "dist.browser/index.js",
"module": "dist.esm/index.js",
"exports": {
  ".": {
    "import": "./dist.esm/index.js",
    "require": "./dist/index.js"
  }
}

Ah, I am just seeing: how do we get our dist.browser build together with this exports section? 🤔

@coderbara
Copy link

I have the same problem. I would like a solution out of the box, without reading the manuals for solving compatibility.

@ryanio
Copy link
Contributor

ryanio commented Apr 5, 2022

I have the same problem. I would like a solution out of the box, without reading the manuals for solving compatibility.

@coderbara Sorry we need to add this to our Common README (I will do this today), the solution is to use require to load the code as @ARJUN-R34 commented:

const Common = require('@ethereumjs/common').default

This is because we have not shipped esm support yet (in progress #1654), so you need to load the code using CommonJS require. Our usage examples are geared toward a TypeScript user with esModuleInterop enabled, which is our recommended development setup.

@ryanio
Copy link
Contributor

ryanio commented Jun 3, 2022

I will be opening an ESM PR for develop today that will solve this issue when released

@holgerd77
Copy link
Member

This should be mitigated by the removal of all default exports in the monorepo along with the breaking releases (VM v6 and others), see #2018, will close.

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

9 participants