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

No default export in Node (immediately overwritten) #36

Closed
niieani opened this issue Jul 22, 2017 · 17 comments
Closed

No default export in Node (immediately overwritten) #36

niieani opened this issue Jul 22, 2017 · 17 comments

Comments

@niieani
Copy link
Owner

niieani commented Jul 22, 2017

Hi @ivanakimov.

The code generated here is quite odd. It proceeds to set the default export as Hashids class, and immediately after overwrites the exports with the same, previously defined default export. The result is that there is no default export for ES6, but only a root CommonJS export.

This will of course work if you use something like babel, which emits an interop between default (ES6) and root exports, but will not work if you consume the code directly, e.g. from Node or TypeScript.

A backwards compatible fix would be to add a 'default' property to the class, which points to itself.

@jd327
Copy link
Collaborator

jd327 commented Jul 26, 2017

I'm using it in Node v8.1.0 and this works:

const Hashids = require('hashids');

How are you using it?

@niieani
Copy link
Owner Author

niieani commented Jul 26, 2017

Yes, that works, because CommonJS' require exposes the module.exports directly.

However, when used from within transpiled code that expects ES6 module APIs, it breaks:

import Hashids from 'hashids'

The above will only work if you use the babel interop that uses either the default export or module.exports fallback.

However, the original code for hashids suggests a default export. According to the spec, this should transpile to:

module.exports.default = Hashids;

The problem with the above however would be that it would break your current syntax:

const Hashids = require('hashids');

i.e. a client would have to explicitly use:

const Hashids = require('hashids').default;

There's a way to support both, though.

module.exports = Object.assign(Hashids, {
  __esModule: true,
  default: HashIds,
})

The culprit seems to be with the choice of the babel plugin, the outdated add-module-exports.

Replacing with transform-es2015-modules-commonjs would solve the problem, but would be a breaking change, since clients would need to do require('hashids').default from within NodeJS.

@MattiLehtinen
Copy link

I had no success with typescript because of this issue. Had to comment out the last line of the library to make it work without errors:

// module.exports = exports['default'];

import Hashids from "hashids";
const hashids = new Hashids("MySalt");

Without the modification I get an error:
TypeError: hashids_1.default is not a constructor

@cormacrelf
Copy link

In Typescript you can still use the const Hashids = require('hashids'); form, in the meantime.

@niieani
Copy link
Owner Author

niieani commented Dec 9, 2017

@cormacrelf only if you target Node. All other forms are broken:

https://codesandbox.io/s/932061k8jw

@cormacrelf
Copy link

@niieani I don’t target node. I use Webpack via @angular/cli. It handles require just fine, minus tree shaking, but this is a one-file module. Try it at the end of that code sandbox.

@niieani
Copy link
Owner Author

niieani commented Dec 9, 2017

@cormacrelf yeah, either Node or if you use Webpack. CodeSandbox doesn't support requires in TypeScript though.

@anisabboud
Copy link
Contributor

I encountered the same issue with Typescript / Angular.

Installed the library and typings via npm:

npm install --save hashids
npm install --save @types/hashids

Importing it as follows results in ERROR TypeError: hashids_1.default is not a constructor:

import Hashids from 'hashids';
const hashids = new Hashids('mysalt');

@cormacrelf's workaround worked for me:

const Hashids = require('hashids');
const hashids = new Hashids('mysalt');

DefinitelyTyped/DefinitelyTyped#15920 might be related.

@mnasyrov
Copy link

My workaround in typescript:

import Hashids, * as HashidsConstructor from 'hashids';
//...
const hashids: Hashids = new (HashidsConstructor as any)();

@supermacro
Copy link

Doesn't

import * as Hashids from 'hashids'

Solve the issue?

@niieani
Copy link
Owner Author

niieani commented Jan 17, 2018

@gDelgado14 no, because typings (and the code) defined a "default" export. Therefore the typings, which are 1:1 with the source code (but not 1:1 with the wrongly transpiled code) expose such a default export. With your code, constructor is available at Hashids, but the typings are exposed at Hashids.default.

@supermacro
Copy link

@niieani

I think the proper declaration file might look something like that of redux-logger.

microsoft/TypeScript#5565 (comment)

@niieani
Copy link
Owner Author

niieani commented Jan 18, 2018

@gDelgado14 yes, changing the TypeScript declaration is one way of fixing the TypeScript issue. But it doesn't change the fact that the transpiled code is nonsensical, caused by the deprecated add-module-exports plugin:

https://github.com/ivanakimov/hashids.js/blob/d208f522ffdd2356655911b90274a625e2262c0a/dist/hashids.js#L374-L375

See my previous comment for a solution #36 (comment).

@jd327
Copy link
Collaborator

jd327 commented Aug 27, 2018

I could use some help with this. What's the best way to set this up without introducing breaking changes?

@niieani
Copy link
Owner Author

niieani commented Aug 31, 2018

@ivanakimov I've fixed the issue in a backwards compatible way in #53.

@jd327 jd327 closed this as completed in 8385a6c Sep 8, 2018
jd327 pushed a commit that referenced this issue Sep 8, 2018
Fix overwritten export (#36) by adding a backwards compatible dist/hashids.js fallback file
@anisabboud
Copy link
Contributor

Thank you Bazyli and Ivan!

The CHANGELOG mentions that v1.2.0 includes the fix, but seems like it hasn't been pushed to npm yet.

@jd327
Copy link
Collaborator

jd327 commented Sep 12, 2018

@anisabboud I didn't do anything, it was all @niieani. New version will go out soon, I just want to give it a few more days in case anyone else spots any issues.

niieani pushed a commit that referenced this issue Aug 15, 2019
Fix overwritten export (#36) by adding a backwards compatible dist/hashids.js fallback file
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

7 participants