microsoft/TypeScript

Inconsistent import behavior would cause `.d.ts` to break.

unional opened this issue · 22 comments

unional commented

TL;DR;

The current inconsistent import behavior in different module mode (system or commonjs) would cause .d.ts to break (1.8 included).

Potential Solution

  • Add a "module {system,commonjs}" directive and so that the .d.ts file will be processed correctly
  • Unify the import/export syntax and behavior
  • .d.ts uses a module agnostic import/export syntax

Context

The context of this problem is about module mapping (such as what jspm and typings supports).

Basically saying any module can be referenced by a different name to avoid module name collision. e.g. in a project that use two versions of jquery, one would be imported normally (import $ from 'jquery'), one would be imported with a different name (import $old from 'jquery@1.6').

In 1.8, with the support of allowSyntheticDefaultImports enables the behavior become similar (but if the user set it to false could still diverge). However the behavior of import * as A from 'abc'; and import dr = require('domready'); are still different.

Problem

The current (1.8) module support is working fine when creating package in TypeScript and distributing it as .js.

However it does not work if the package is distributed in .ts or with .d.ts.

The reason is that when distributing in the compiled form, the module mode used by the package author is captured in the .js file. When distributing .ts and .d.ts it relies on the consumer to use the same module mode as the package author intended.

This means if the package authors (including all typings authors) create the package using one module mode, and the consuming application/package use a different module mode, the system will fail.

It is worse if the consuming application/package uses multiple packages with disagreeing module mode.

Here are the original discussion:
typings/typings#149

This is closely related to:
#7125

Please let me know if there are any confusion or missing information / context.

unional commented

In similar context the .d.ts should not be written with declare module 'abc'. It should be written in "external module format" described in typings.
i.e. Suggestion in this https://github.com/Microsoft/TypeScript-Handbook/blob/6ba6e27b0b918160adfb4e52bf528ecd1467f62c/pages/Modules.md#ambient-modules might not be a good advice

We could define each module in its own .d.ts file with top-level export declarations, but it's more convenient to write them as one larger .d.ts file. To do so, we use a construct similar to ambient namespaces, but we use the module keyword and the quoted name of the module which will be available to a later import.

The term "external module format" is not exact (and not align with TypeScript to my understanding). But that's YET another topic. 🌹

mhegazy commented

@unional can you provide an example of the issue. i am having hard time understand the underlying problem.

unional commented

Say I have a commonjs foo module:

// foo module
module.export = function foo() {}

I can import foo module into boo using the following syntax:

// boo module
import foo from 'foo';
import * as foo2 from 'foo';

Currently, depends on the value of module and allowSyntheticDefaultImports, the behavior of the above two statements could be different.

So the problem is this. If I distribute the boo module with typescript file, either as boo.ts or boo.js + boo.d.ts, the module and allowSyntheticDefaultImports value I have made is not captured in those file.

i.e. when an app consumes boo, and app's module and allowSyntheticDefaultImports does not match what is in boo, then the resolution will fail.

The problem cannot be solved in app because if app consumes boo and boo2, which assume different module and allowSyntheticDefaultImports value, the resolution will fail one way or the other.

Does this help?

mhegazy commented

thanks, this is helpful.

this is correct. the problem here is not allowSyntheticDefaultImports; the problem is the loaders.

CommonJS has a concept of module.exports = function, this is what TypeScript modules using export=. ES6 spec did not include a similar concept. modules are always objects, and can not be functions, instead they added a default export.

ES6 module loader implementation, decided to add a shim for these, using the default export. so for instance, es6-module-loader, used by Systemjs, will have the module exports all under the default property, and will copy then out to the module as well. making allowing import foo from 'foo' and import * as foo2 from 'foo'; to work for getting exports, but import foo from 'foo'; for calling foo() as a function.

keep in mind that other loaders, i.e. commonjs has no idea of a default import.

So in short, how you can consume the module depends on what loader you are using.

--allowSyntheticDefaultImports mimics what systemJS does, and it is on by default for compilations with --module system.

if your loader is not doing this copying, do not enable this flag.

does this cover the OP?

unional commented

the problem here is not allowSyntheticDefaultImports;

Agree, and it actually helps to narrow the gap.

My concern in this post is not the usage of allowSyntheticDefaultImports as a consumer. But the mismatch between the expected loader and configuration between the package author (who consumes other packages) and the consumer.

As you said "the problem is the loaders", the consequence it that if any package author starts to:

  • author package in system, or
  • author package in commonjs with allowSynthethicDefaultImports enabled

And then the consumer use it in commonjs with allowSynthethicDefaultImports disabled.

Or in other combinations that the package author's intent of import foo from 'foo' and import * as foo from 'foo' does not align with the consumer's configuration.

Would umd able to solve this? or it is still a partial solution?

mhegazy commented

Or in other combinations that the package author's intent of import foo from 'foo' and import * as foo from 'foo' does not align with the consumer's configuration.

I am not sure i see what you mean here. Either the package author has a default export or they do not. if they do, then this discussion is not relevant, if they do not, the importer needs to decide how to import it, either they import using import * as foo from "foo" or import it as a default, and specify the flag.

Would umd able to solve this? or it is still a partial solution?

I do not see how UMD would solve this issue.

unional commented

It's not what they export, it's what they import. i.e.

// moduleA
module.export = foo() {}
// moduleB
import foo from 'moduleA' 
// or `import * as foo from 'moduleA'`, 
// depends on moduleB's `module` and `allowSynthethicDefaultImports`

export default function boo() { foo(); }

export foo; // re-export or extends it.
// appC
import boo from 'moduleB'
// do something

If appC's config (module and allowSynthethicDefaultImports) is differ then moduleB's config, we will have problem in the moduleB.d.ts file

UPDATED sample

mhegazy commented

i see.
so what is the proposal?

unional commented

so what is the proposal?

Honestly, don't know~~~ 😜

What I can think of are these:

  1. Add a "module {system,commonjs}" directive and so that the .d.ts file will be processed correctly
  2. Unify the import/export syntax and behavior
  3. .d.ts uses a module agnostic import/export syntax

Especially 1. But I don't know if that is possible because it involves multiple loaders.

DanielRosenwasser commented

If appC's config (module and allowSynthethicDefaultImports) is differ then moduleB's config, we will have problem in the moduleB.d.ts file

Sorry, could you clarify what problem you will have in moduleB.d.ts?

unional commented

The resolution will get the wrong thing. E.g. foo() vs { default: foo() }

Thus the types used in moduleB.d.ts will mess up.

unional commented

Same thing (or worse?) applies if the module is distributed in .ts instead of in compiled .js

unional commented

This is not exactly a TypeScript issue. It is really a issue of the interop between ES6 and commonjs.

It is just that the effect impact TypeScript first because of the distribution of the declaration files.

When people start to distribute package in raw ES6 and it consumes nodeJS package......

.....it sounds like a systematic crisis. :(

I hope I'm wrong.

Maybe you can invite members of the loaders and ES6 to discuss this?

unional commented

Just to complete the story (as you know very well already), not only the ES6 syntax is affected:

// module = "commonjs", allowSynthethicDefaultImports = true | false
import tape = require('blue-tape');

isFunction(tape);
isUndefined(tape.default);

// module = "system"
import tape = require('blue-tape');
!isFunction(tape);
isImmutableObject(tape);
isFunction(tape.default);
DanielRosenwasser commented

Perhaps the correct thing to do is to not respect allowSyntheticDefaultImports in ambient contexts, but that would be a break.

unional commented

However, the above case is still inconsistent no matter what the allowSyntheticDefaultImports value is

unional commented

Also, non-ambient (source code) also has this issue if they are distributed. Although currently it is rare.

unional commented

What do you think about the module directive proposal?

  1. If "module system" exists on the .d.ts or .ts file, tsc will use that module loader to resolve the import variable.
  2. Extends package.json/typings: <filepath> | { "module": "system | commonjs": "file": <filepath>" }
  3. If the package distributes tsconfig.json with it, honor the module and allowSyntheticDefaultImports in it.
unional commented

By the way, (2) and (3) doesn't work with https://github.com/typings/typings as there is no package.json nor tsconfig.json

unional commented

I'm seeing this starts to take effect in live code, not just typings files. The codebase starts to divide.
What can be done to get TS and JS (Babel) back together?

For example, currently, in order to use redux-logger in TS, you need to:

import * as createLogger from 'redux-logger'

instead of

import createLogger from 'redux-logger'

as suggested in its document: https://github.com/evgenyrodionov/redux-logger

Although the source of redux-logger do export default createLogger

If I use allowSyntheticDefaultImports to get around this issue, then my consumer will get into the situation I described here.