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

Inconsistent import behavior would cause .d.ts to break. #7398

Open
unional opened this issue Mar 5, 2016 · 22 comments
Open

Inconsistent import behavior would cause .d.ts to break. #7398

unional opened this issue Mar 5, 2016 · 22 comments
Labels
Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. Suggestion An idea for TypeScript

Comments

@unional
Copy link
Contributor

unional commented Mar 5, 2016

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 '[email protected]').

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
Copy link
Contributor Author

unional commented Mar 5, 2016

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. 🌹

@unional
Copy link
Contributor Author

unional commented Mar 5, 2016

cc @blakeembrey

@mhegazy
Copy link
Contributor

mhegazy commented Mar 29, 2016

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

@mhegazy mhegazy added the Needs More Info The issue still hasn't been fully clarified label Mar 29, 2016
@unional
Copy link
Contributor Author

unional commented Mar 29, 2016

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
Copy link
Contributor

mhegazy commented Mar 29, 2016

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
Copy link
Contributor Author

unional commented Mar 29, 2016

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
Copy link
Contributor

mhegazy commented Mar 30, 2016

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
Copy link
Contributor Author

unional commented Mar 30, 2016

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
Copy link
Contributor

mhegazy commented Mar 30, 2016

i see.
so what is the proposal?

@unional
Copy link
Contributor Author

unional commented Mar 30, 2016

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
Copy link
Member

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
Copy link
Contributor Author

unional commented Mar 30, 2016

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
Copy link
Contributor Author

unional commented Mar 30, 2016

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

@unional
Copy link
Contributor Author

unional commented Mar 30, 2016

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
Copy link
Contributor Author

unional commented Mar 30, 2016

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
Copy link
Member

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

@DanielRosenwasser DanielRosenwasser added Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. and removed Needs More Info The issue still hasn't been fully clarified labels Mar 31, 2016
@unional
Copy link
Contributor Author

unional commented Mar 31, 2016

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

@unional
Copy link
Contributor Author

unional commented Apr 1, 2016

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

@unional
Copy link
Contributor Author

unional commented Apr 9, 2016

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
Copy link
Contributor Author

unional commented Apr 9, 2016

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
Copy link
Contributor Author

unional commented Jan 24, 2017

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.

@unional
Copy link
Contributor Author

unional commented Mar 16, 2017

@RyanCavanaugh RyanCavanaugh added Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. and removed Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. labels Jul 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

4 participants