Added namespace to allow es6 style import

[CaselIT]

Added empty package to allow es6 style import in typescript

import * as auth from 'basic-auth';

[felixfbecker]

This is not correct according to ES6 spec. If you import *, according to the spec, you will not get a function, but a plain object. Your options are:

import auth = require('basic-auth')

this works with module: 'commonJS'

or

import auth from 'basic-auth'

this will work with module: ES6 and allowSyntheticDefaultExport: true. You will have to then use a module loader like SystemJS that supports this or transpile again with Babel.

[CaselIT]

Well a function is an object in JavaScript.
This is also used in other libraries. One among many is express.

import * as express from 'express';
const app = express();

The return value is a function.

[felixfbecker]

I said plain object. In ES6:

import * as x from 'express'
console.log(typeof x)

will log object, not function. TypeScript is simply not following the spec here.
I can tell you for sure that this incorrect usage is not used in the express typings.
Furthermore, I will not merge a hack to support this incorrect usage - I gave you two examples of correct usage :)

[CaselIT]

Compiling that code with Typescript, the console will log function.
But I understand your point.

Will wait for basic-auth to update to an es6 style export, if ever

[felixfbecker]

Why can't you use one of the examples I gave you?

[blakeembrey]

@CaselIT import * is not an object in ES6, but rather bindings (see http://www.2ality.com/2015/07/es6-module-exports.html). That style just happens to work in TypeScript when compiled into CommonJS, which has been quoted as a potential bug in the same issue many link to as the recommendation for this hack. I've also been rejecting these PRs personally, and it's a real pain that DefinitelyTyped promoted such behaviour by merging these hacks in the past. As such, we aren't merging hacks like this - instead you should prefer CommonJS imports for CommonJS code. Here's a recent comment on DefinitelyTyped which also sums it up - DefinitelyTyped/DefinitelyTyped#11479 (comment).

Linked in that comment there's a good post by Ryan on Stack Overflow too: https://stackoverflow.com/questions/39415661/why-cant-i-import-a-class-or-function-with-import-as-x-from-y. I'd recommend bookmarking this for future use.

[CaselIT]

@felixfbecker

Why can't you use one of the examples I gave you?

I meant that to use the es6 style import will wait for the library to be updated. I resorted to the es5 style, but I dislike it because it'd different from almost all other imports I use.

@blakeembrey thanks as always for the explanation. I had a feeling that the CommonJS import style in typescript was almost deprecated, or in any case the es6 style import was to be preferred (but that's just my impression, by seeing the es6 style almost everywhere, and is not supported by any official evidence so to speak). I personally find the es6 version more convenient in many cases.

Just a curiosity, using basic-auth as an example:
if the library were to be updated form

module.exports = auth

to

module.exports = auth;
module.exports.default = auth;

would this be acceptable as a way to use the es6 import import auth from "basic-auth" ?

Or it's more of an hack that should be avoided?
Sorry if it seem obvious, but I still find some of the aspects of the js world a bit puzzling.

[felixfbecker]

@CaselIT Yes, I promote that synthetic default export a lot.
https://github.com/sequelize/sequelize/blob/master/lib/sequelize.js#L1280-L1282
https://github.com/types/npm-sequelize/blob/master/4/lib/sequelize.d.ts#L256
https://github.com/types/npm-sequelize/blob/master/4/lib/sequelize.d.ts#L1276

I agree that the module.exports / export = / import = require() is ugly and not forward-compatible, but it is the only correct way

[CaselIT]

@felixfbecker Very well. I'll make a pr to basic-auth to introduce it.

Thanks for the clarification about how to best write type definitions.

[unional]

Have to jump in the discussion. Really need to use import x = require('y') for commonjs:
https://github.com/unional/typescript-guidelines/blob/master/pages/default/modules.md#import-keyword
microsoft/TypeScript#7398

EDIT: the issue above relates to export default, not on the module namespace object, but it is best to educate and just use CommonJS as what it is, do not rely on interop.

[CaselIT]

@unional If the commonjs library uses module.exports.default, that issue should not arise. Or am I missing something?

[unional]

That should work; however, I would recommend not to do so because attempting to blur the line between CommonJS and ES2015 this way causes confusion to the team/user.

It gives a wrong impression that interop can safely be used. My two cents. 🌷

[blakeembrey]

I'm also 90% confident it would break when real ES6 modules are released. We would have to wait until the latest node.js ESM proposal is available though, it sounds like they may have changed/fixed some of these issues. There's not a lot going for a module author to add that code unless they are also controlling the consumption of .d.ts files as well (E.g. wrote it in TypeScript themselves), as the maintenance cost is really only a burden on them. They would need to release a version now, and probably another in the near future depending on ecosystem changes (currently unknown which changes, so you can't avoid this) as a major version since changing the .default property after it was added would be a breaking change.

[CaselIT]

@blakeembrey Why do you think that would break? Isn't the spec for the es6 modules final?

[unional]

Agree with Blake. You can check node-eps

It is best to avoid interop and anything resemble it right now

[unional]

Es6 spec does not specify interop

[CaselIT]

From node-eps

all properties of module.exports will be hoisted to named exports after evaluation of CJS modules with the exception of default which will point to module.exports directly.

would suggest that the default import would continue to work, but not the namespace hack I was proposing to use in the PR. Also see the 3rd example

[blakeembrey]

@CaselIT The ES6 module spec is, yes, but even today you are not using that spec. No one supports it in production. You are using a transpiled version to CommonJS or AMD.

[blakeembrey]

@CaselIT That example which is quote is not compatible with this change. What TypeScript is doing when you do import x from 'foo' is using module.exports.default. What the EPS is suggesting is import x from 'foo' uses module.exports directly. You might want to briefly read over the spec more, but don't get confused that a property can be named default while ES6 has a concept of a "default" export and in this case module.exports is being treated like export default which means the example you're looking at is export default { default: ... } (two layered).

Edit: To clarify, actually, this code works accidentally still because of the fact that module.exports === module.exports.default. But it doesn't make a compelling case to make the change. Also, it's probable there'll be a new proposal soon from node.js - see https://twitter.com/mikeal/status/781530255198523392.

[blakeembrey]

Note that the current master isn't final either. See above, or nodejs/node-eps#39 or nodejs/node-eps#43.

[CaselIT]

Could be, but it's not so very clear.
If it was as you suggest the part with the exception of default would not have sense.

It should have been something like

all properties of module.exports will be hoisted to named exports after evaluation of CJS modules. default will point to module.exports directly.

Still since it's s draft, who knows what will actually be the implementation. And if ever there will be one