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

TypeScript definition CommonJS compatibility #1408

Closed
wants to merge 1 commit into from
Closed

Conversation

hayeah
Copy link

@hayeah hayeah commented Mar 1, 2018

TypeScript compiler generates "web3.default" to access the export default value. However the underlying CommonJS module does not have a "default" property.

TypeScript has supports the CommonJS module export pattern with this syntax:

// module.exports = foo
export = foo

@coveralls
Copy link

coveralls commented Mar 1, 2018

Coverage Status

Coverage remained the same at 85.832% when pulling bbda145 on hayeah:patch-1 into c9ca65d on ethereum:1.0.

@danakt
Copy link

danakt commented Mar 1, 2018

It should look like this:

namespace Web3 {}
export = Web3

Otherwise, during import, typescript displays the following error: Module ''web3'' resolves to a non-module entity and cannot be imported using this construct.

@hayeah
Copy link
Author

hayeah commented Mar 1, 2018

hmm it seems to work for me. here's my tsconfig

{
  "compilerOptions": {
    /* Basic Options */
    "target": "ES2017", /* Specify ECMAScript target version: 'ES3' (default), 'ES5', 'ES2015', 'ES2016', 'ES2017', or 'ESNEXT'. */
    "module": "commonjs", /* Specify module code generation: 'commonjs', 'amd', 'system', 'umd', 'es2015', or 'ESNext'. */
    // "lib": [],                             /* Specify library files to be included in the compilation:  */
    // "allowJs": true,                       /* Allow javascript files to be compiled. */
    // "checkJs": true,                       /* Report errors in .js files. */
    "jsx": "react", /* Specify JSX code generation: 'preserve', 'react-native', or 'react'. */
    "declaration": true, /* Generates corresponding '.d.ts' file. */
    // "sourceMap": true,                     /* Generates corresponding '.map' file. */
    // "outFile": "./",                       /* Concatenate and emit output to single file. */
    "outDir": "lib", /* Redirect output structure to the directory. */
    // "rootDir": "./",                       /* Specify the root directory of input files. Use to control the output directory structure with --outDir. */
    // "removeComments": true,                /* Do not emit comments to output. */
    // "noEmit": true,                        /* Do not emit outputs. */
    // "importHelpers": true,                 /* Import emit helpers from 'tslib'. */
    // "downlevelIteration": true,            /* Provide full support for iterables in 'for-of', spread, and destructuring when targeting 'ES5' or 'ES3'. */
    // "isolatedModules": true,               /* Transpile each file as a separate module (similar to 'ts.transpileModule'). */
    /* Strict Type-Checking Options */
    "strict": true, /* Enable all strict type-checking options. */
    "skipLibCheck": true,
    // "noImplicitAny": true,                 /* Raise error on expressions and declarations with an implied 'any' type. */
    // "strictNullChecks": true,              /* Enable strict null checks. */
    // "noImplicitThis": true,                /* Raise error on 'this' expressions with an implied 'any' type. */
    // "alwaysStrict": true,                  /* Parse in strict mode and emit "use strict" for each source file. */
    /* Additional Checks */
    // "noUnusedLocals": true,                /* Report errors on unused locals. */
    // "noUnusedParameters": true,            /* Report errors on unused parameters. */
    // "noImplicitReturns": true,             /* Report error when not all code paths in function return a value. */
    // "noFallthroughCasesInSwitch": true,    /* Report errors for fallthrough cases in switch statement. */
    /* Module Resolution Options */
    "moduleResolution": "node", /* Specify module resolution strategy: 'node' (Node.js) or 'classic' (TypeScript pre-1.6). */
    // "baseUrl": "./",                       /* Base directory to resolve non-absolute module names. */
    // "paths": {},                           /* A series of entries which re-map imports to lookup locations relative to the 'baseUrl'. */
    // "rootDirs": [],                        /* List of root folders whose combined content represents the structure of the project at runtime. */
    // "typeRoots": [],                       /* List of folders to include type definitions from. */
    // "types": [],                           /* Type declaration files to be included in compilation. */
    // "allowSyntheticDefaultImports": true,  /* Allow default imports from modules with no default export. This does not affect code emit, just typechecking. */
    /* Source Map Options */
    // "sourceRoot": "./",                    /* Specify the location where debugger should locate TypeScript files instead of source locations. */
    // "mapRoot": "./",                       /* Specify the location where debugger should locate map files instead of generated locations. */
    // "inlineSourceMap": true,               /* Emit a single file with source maps instead of having a separate file. */
    // "inlineSources": true,                 /* Emit the source alongside the sourcemaps within a single file; requires '--inlineSourceMap' or '--sourceMap' to be set. */
    /* Experimental Options */
    "experimentalDecorators": true /* Enables experimental support for ES7 decorators. */
    // "emitDecoratorMetadata": true,         /* Enables experimental support for emitting type metadata for decorators. */
  },
  "include": [
    "src/**/*.ts",
    "src/**/*.tsx",
    "src/**/*.d.ts"
  ]
}

@danakt
Copy link

danakt commented Mar 1, 2018

Strange, because this is a known problem. Look this: microsoft/TypeScript#5073 (comment)

@hayeah
Copy link
Author

hayeah commented Mar 1, 2018 via email

@danakt
Copy link

danakt commented Mar 1, 2018

Unfortunately, this applies to classes too

@dpyro
Copy link

dpyro commented Mar 1, 2018

I just pushed the exact same diff for the exact same reason! 😂

@danakt In typescript export = Web3 must be consumed as import Web3 = require('web3'). This fixes the error you described. import * as Web3 from 'web3' is usually preferred but only works when the exported object is a namespace and not a usable function.

@dpyro
Copy link

dpyro commented Mar 10, 2018

This was very painful for me to handle in a portable manner short of publishing my own packages. Workaround:

Run:

npm install --save-dev replace-in-file

Add to package.json:

"scripts": {
    "postinstall": "replace-in-file 'export default' 'export = ' ./node_modules/web3/index.d.ts"
}

@frozeman
Copy link
Contributor

Ok so whats the consensus on this one?

@hayeah
Copy link
Author

hayeah commented Mar 14, 2018

@frozeman As @dpyro mentioned above, the following syntax would work:

import Web3 = require('web3')

@ShiningRay
Copy link

module.exports.default = Web3;

@frozeman
Copy link
Contributor

So im still confused...

@jpeletier
Copy link
Contributor

You can't export two things. Just leave export = Web3. If you want, I can open a PR with the replacement file we're using in all of our projects.

@frozeman
Copy link
Contributor

The OP should make the proper fixes, or you can make another PR and we close this one. Im dont use typescript so i cant tell whats right.

@hayeah
Copy link
Author

hayeah commented Apr 12, 2018

To muddy water perhaps a bit more, contrary to @jpeletier's experience, my version of typescript supports exporting both:

export default Web3;
export = Web3;

This is too confusing.

Could we do what @ShiningRay suggested and just change the JS source, and close this PR? Add in JS:

module.exports.default = Web3;

@dpyro
Copy link

dpyro commented Apr 12, 2018

The ES6 class syntax deprecates the CommonJS style export module.exports = in favor of named exports or a export default for all transpilers/loaders/etc. For that reason I recommend that that index.js in 1.0ES6 is changed from module.exports = to export default whenever the next breaking change is allowed.

@frozeman
Copy link
Contributor

Hm, in fact we need to update the 1.0ES6 branch, its a bit behind 1.0, if anybody wants to take that on would be great.

@nivida
Copy link
Contributor

nivida commented Nov 29, 2018

Because the ongoing refactoring (#2000) will fix this issue I'm closing this PR.

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

Successfully merging this pull request may close these issues.

8 participants