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

Synthesize namespace records on CommonJS imports if necessary #16093

Closed
DanielRosenwasser opened this issue May 26, 2017 · 53 comments · Fixed by #19675
Closed

Synthesize namespace records on CommonJS imports if necessary #16093

DanielRosenwasser opened this issue May 26, 2017 · 53 comments · Fixed by #19675
Assignees
Labels
Committed The team has roadmapped this issue Domain: ES Modules The issue relates to import/export style module behavior Fixed A PR has been merged for this issue Suggestion An idea for TypeScript

Comments

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented May 26, 2017

TL;DR

Flags like --allowSyntheticDefaultImports will just work without extra tools like Webpack, Babel, or SystemJS.

TypeScript will just work with the --ESModuleInterop flag without extra tools like Webpack, Babel, or SystemJS.

See the PR at #19675 for more details.

Background

TypeScript has successfully delivered ES modules for quite some time now. Unfortunately, the implementation of ES/CommonJS interop that Babel and TypeScript delivered differs in ways that make migration difficult between the two.

TypeScript treats a namespace import (i.e. import * as foo from "foo") as equivalent to const foo = require("foo"). Things are simple here, but they don't work out if the primary object being imported is a primitive or a value with call/construct signatures.
ECMAScript basically says a namespace record is a plain object.

Babel first requires in the module, and checks for a property named __esModule. If __esModule is set to true, then the behavior is the same as that of TypeScript, but otherwise, it synthesizes a namespace record where:

  1. All properties are plucked off of the require'd module and made available as named imports.
  2. The originally require'd module is made available as a default import.

This looks something like the following transform:

// Input:
import * as foo from "foo";

// Output:
var foo = __importStar(require("foo"));

function __importStar(mod) {
    if (mod && mod.__esModule) return mod;
    var result = {};
    if (mod != null) {
        for (var k in mod) {
            if (Object.prototype.hasOwnProperty.call(mod, key)) {
                result[k] = mod[k]
            }
        }
    }
    result.default = mod;
    return result;
}

As an optimization, the following are performed:

  1. If only named properties are ever used on an import, *the require'd object is used in place of creating a namespace record.

    Note that this is already TypeScript's behavior!

  2. If

    1. a default import is used at all in an import statement and
    2. if the __esModule flag is not set on the require'd value, then

    A fresh namespace record whose default export refers to the require'd value will be created in place of the require'd value.
    In other words:

    // This TypeScript code...
    import { default as d } from "foo";
    d.member;
    
    // Would become this CommonJS JavaScript code...
    var foo = require("foo");
    var foo_2 = foo && foo.__esModule ? foo : {default: foo};
    foo_2.default.member;

Note that there is no optimization for never using the default. This is probably because you'd need alias analysis and could never be sure that someone else would use the default.

Proposal

I believe that we'd serve both communities well if we decided to adopt the aforementioned emit.

Drawbacks

Performance & Size Impact

This certainly impacts the size and readability of TypeScript's output.
Furthermore, for large CommonJS modules, there could be a speed impact to keep in mind - notice that these synthesized namespace records are not cached at all.

Tools already do this for us

Those who've been using tools like Webpack 2 and SystemJS have been getting this functionality for a few months now.
For example, if you target ES modules, Webpack 2 will already perform this behavior.
Taking this strategy makes it an opt-in for users who care about the interop behavior.

I mainly bring this up because I want to immediately bring up the counterpoint.
While these tools are definitely central to many modern web stacks, they won't cover

  1. Vanilla Node.JS users
  2. React Native Packager users
  3. Browserify users (duh)
  4. Users whose test code doesn't run through these tools
@DanielRosenwasser DanielRosenwasser added In Discussion Not yet reached consensus Suggestion An idea for TypeScript labels May 26, 2017
@DanielRosenwasser
Copy link
Member Author

DanielRosenwasser commented May 26, 2017

cc @TheLarkInn @hzoo @guybedford

@aluanhaddad
Copy link
Contributor

This is fantastic.
I really want to see this happen.
Thank you very much.

I want to add that one benefit of this is that it simplifies documentation, tutorials, and other learning materials that currently either have long explanations regarding these differences or, as is more often the case, only show an import usage example that works for either the TypeScript community or for the Babel community but not for both.

@guybedford
Copy link
Contributor

guybedford commented May 26, 2017

This is great to hear and I'm really glad TypeScript is looking to support this interop.

@DanielRosenwasser do you perhaps mean to correct if the __esModule flag is set on the require'd value, then to read if the __esModule flag is not set on the require'd value, then above?

Just to share the current shift SystemJS has had to make wrt interop - I know this has probably been discussed ad nauseam, but just to voice my opinion again...

Namespace lifting for CommonJS

NodeJS doesn't seem to have any plans of treating CommonJS modules as namespaces, and instead simply providing them as default exports, due to all the edge case concerns here of calling getters etc. While I know this is gambling on a possibility, I think it is worse to gamble that NodeJS will get over this restriction any time soon in its adoption of ES modules. SystemJS took this hit already (while using __esModule as a fallback method), and it has definitely affected users.

Just thought it is worth mentioning as I do think it would benefit TypeScript to double down on the syntax import x = for CJS as this allows a great way to syntactically separate CJS requires and their named exports, without assuming that CommonJS modules can be magically converted into ES module namespaces. import x from 'x' for CJS can still be allowed, while gradually deprecating import {x} from 'cjs'.

Edit: and perhaps Node core modules can be treated as exceptions here due to the expectation that in the conversion to ES modules, NodeJS core would permit named exports for these.

@aluanhaddad
Copy link
Contributor

aluanhaddad commented May 26, 2017

Just thought it is worth mentioning as I do think it would benefit TypeScript to double down on the syntax import x = for CJS as this allows a great way to syntactically separate CJS requires and their named exports, without assuming that CommonJS modules can be magically converted into ES module namespaces. import x from 'x' for CJS can still be allowed, while gradually deprecating import {x} from 'cjs'.

Just a thought, but I wonder if it might be worth adding another --module target to TypeScript, say CommonJSStrict, that only supports the set of operations that Node currently plans to support for interop.
I realize this would add complexity to TypeScript, but it might be easier than deprecating named imports. If Node adopts named imports, then that feature could be added to the new module target.

I've been recommending the import = syntax to people targeting CommonJS for a while now.

Also, CC @unional, @Kovensky

@unional
Copy link
Contributor

unional commented May 26, 2017

Thanks @aluanhaddad, for notifying me about this wonderful issue. Why can't we add more than one 👍 to the OP? 🌷

I have also been recommending the import = syntax for a while. It works nicely in my use cases.

However, I want to bring up that unfortunately, it cannot be a long-term solution. The issue is browsers' eventual support of ESM.

When user target es6, the import = needs to be interop to import default as the require() function is not available in the browser without the help of any loader.

This means CJS -> ESM interop and a unify view of CJS <-> ESM is still needed. And it also means magically converting CJS -> ESM (for the dependencies of the compiling package) is still needed to be performed during compilation.

(edit: we should point this out to the nodejs folks as their decision on this prohibits code sharing on server and browser unless all dependencies are ESM)

Another major issue needs to be dealt with is typings and direct ts source code distribution.
As mentioned in #7398, the implicit capturing of the tsconfig in the typings is very problematic and could greatly complicate the implementation of this work and the adaptation of the new mode CommonJSStrict if introduced.
We should also look at that one to see if there is a solution to that or a migration path on updating all typings available.
Speaking of which, making typings to respect scope would be desirable, but that's another issue. 🌷

@unional
Copy link
Contributor

unional commented May 26, 2017

@DanielRosenwasser
Copy link
Member Author

@unional I think that the .d.ts issue is something we'll have to deal with separately down the line. I've already been pushing new PRs in DefinitelyTyped that describe value-exported entities to use the export = syntax, and use import x = require('...') syntax.

@DanielRosenwasser do you perhaps mean to correct if the __esModule flag is set on the require'd value, then to read if the __esModule flag is not set on the require'd value, then above?

@guybedford Yes, thanks, I corrected my post.

Just thought it is worth mentioning as I do think it would benefit TypeScript to double down on the syntax import x = for CJS as this allows a great way to syntactically separate CJS requires and their named exports

@guybedford I agree, but unfortunately, a lot of people are extremely turned off by that idea for philosophical reasons. The existence of import foo = require('...') makes complete sense and is actually useful, but my experience is people don't want to hear it.

Just a thought, but I wonder if it might be worth adding another --module target to TypeScript, say CommonJSStrict

@aluanhaddad that's something @mhegazy and I have discussed at some point, and I think it'll be on the table, but since nobody really knows what will happen in 6 months time, I think we should wait. At the very least, adopting this behavior will mean that there is a smooth migration path for users (as opposed to now where users can't get ready for a change like that at all without an external tool).

@DanielRosenwasser
Copy link
Member Author

DanielRosenwasser commented May 26, 2017

After our design meeting today, I don't think this is 2.4-bound, but what we're looking for is the best migration path we can conceive, along with a story that synchronizes with type declarations on DefinitelyTyped.

Current concerns are that the Babel emit does not allow namespace imports to be callable, which actually does break a lot of (technically invalid) code, such as the following:

import * as padLeft from "pad-left";
padLeft(10, "zup doug");

So a potential modification could be the following

function __importStar(mod) {
    if (mod && mod.__esModule) return mod;

    // For callable/constructable things, tack properties onto a function record
    // that passes arguments through to the actual import
    var result = !(mod instanceof Function) ?
        {} :
        function() { return mod.apply(this, arguments); };

    if (mod != null) {
        for (var k in mod) {
            if (Object.prototype.hasOwnProperty.call(mod, key)) {
                result[k] = mod[k]
            }
        }
    }
    result.default = mod;
    return result;
}

But we might also just be willing to have a new mode that goes with Babel's emit and breaks away from the old behavior in that respect.

@Jessidhia
Copy link

The behavior of import { x, y, z } from 'foo' is only correct if foo is an ES module. If it is a commonjs-exported object, it should look like a namespace with only a default export (i.e. x, y and z should be undefined regardless of whether foo's module.exports has those keys, and ideally be build-time errors).

All non-star-import emits should behave as if a hidden import * as _ref from 'cjs' was done; with the this value erased if the direct imports are called as a function; e.g. importedDefault() -> (0, _ref.default)(). The star import is basically imported.__esModule ? imported : Object.freeze({ __proto__: null, [Symbol.toStringTag]: 'Module', default: imported }).

Any of the changes to make TypeScript any closer to ESM/CJS interoperation spec compliance will be breaking because of how off the TypeScript implementation is from the actual proposed interoperation semantics. This would also need stricter semantics than --allowSyntheticDefaults -- a star import from CJS is a namespace with a default export, not the CJS exports with a self-referencing default.

@felixfbecker
Copy link
Contributor

A lot of libraries have started to do stuff like this:

module.exports = BigNumber
module.exports.BigNumber = BigNumber
module.exports.default = BigNumber

To support nicer ES6 imports à la

import { BigNumber } from 'bignumber.js'
import BigNumber from 'bignumber.js'

the majority of libraries is not using a transpiler and will have to support CommonJS/Node imports, so it would be very sad if this is broken

@guybedford
Copy link
Contributor

As an update on #16093 (comment), note that NodeJS has an ES module implementation PR at nodejs/node#14369 with no plans to support synthetic exports at all. This implementation will likely be released early next year sometime. TypeScript has the ability to start preparing for the impending break of losing named exports for CJS here already.

@guybedford
Copy link
Contributor

guybedford commented Aug 4, 2017

As a follow up, has it been considered before to support a destructuring variation of the import x = require('x') pattern in TypeScript?

Something like:

import { destructuredExport } = require('pkg');

as sugar for const { destructuredExport } = require('pkg') in CommonJS and import _pkg from 'pkg'; const { destructuredExport } = _pkg in ES modules.

That could complete the syntax on this form making it a syntax specific way of indicating this interop pattern.

Come on TS team, bite this bullet before it bites you :)

@frankwallis
Copy link
Contributor

In the interim would it not be possible to remove the TS1202 warning

TS1202: Import assignment cannot be used when targeting ECMAScript 2015 modules. Consider using 'import * as ns from "mod"', 'import {a} from "mod"', 'import d from "mod"', or another module format instead.

And output

const mymod = require('some-module');

as that is in fact what the user asked for?

Currently it is not possible to target both es2015 and commonjs when importing a library which exports itself as a commonjs module. This is a common scenario when developing UI code which is unit tested in nodejs and bundled for the browser using webpack/rollup. In order to support nodejs it is necessary to compile to commonjs, but in order to take advantage of tree-shaking and module hoisting when bundling it is necessary to compile to es2015 modules.

It is not possible to use import mymod from 'some-module' as this code will fail at runtime in nodejs due to the lack of interopRequireDefault.

The previous workaround of using import * as add from 'some-commonjs-module' and adding a namespace to the declaration file is now being disallowed in DefinitelyTyped which means that the only solution is to use const mymod = require('some-module') and lose all the typing.

@DanielRosenwasser
Copy link
Member Author

as that is in fact what the user asked for?

@frankwallis Yes, if it was known that the user expected that to emit to CommonJS, but not if they were asking for AMD or SystemJS.

@frankwallis
Copy link
Contributor

@DanielRosenwasser thanks I see the problem now. Although I am yet to be convinced that there is a real-world use-case for mixing ES2015 imports and amd/commonjs...

It seems to me that resolving ES2015/CommonJS interop could be a long drawn-out process, and in the meantime this is causing real problems. Perhaps a better interim solution could be to output import * as mymod from 'some-module' when compiling to ES2015 as both Webpack and SystemJS do handle this (according to the initial post on this thread)

@guybedford
Copy link
Contributor

A module written with import x = require('x') should become import x from 'x' in ES 2015 output and this should really be the recommended way to ensure correct interop. I'm not sure what I'm missing here?

@frankwallis
Copy link
Contributor

@DanielRosenwasser is there anything to prevent typescript from compiling

import mymod = require('mymodule')

to

import mymod from 'mymodule'

when outputting ES2015 modules?

[Optionally this behaviour could be controlled by the allowSyntheticDefaultImports flag]

@felixfbecker
Copy link
Contributor

@frankwallis @guybedford Imo that doesn't make sense. CommonJS has a default export, ES6 has a default export. If you do

import mymod = require('mymodule').default

it should become

import mymod from 'mymodule'

as that is only shorthand for

import {default as mymod} from 'mymodule'

if you do

import mymod = require('mymodule')

it should be become

import * as mymod from 'mymodule'

as it is today.

@Jessidhia
Copy link

And that is why this whole situation is a mess. It should not as (in the actual commonjs interop spec that was not written at the time TS implemented modules) the module.exports value is only visible as the default export, no matter what keys are in that object (or not object; exports can be number, string or undefined too).

The mymod in your * as mymod example, in real modules, would be a frozen object (similar, but not quite the same, as Object.freeze’s effect) that looks like { [Symbol.toStringTag]: “Module”, default: require(“mymodule”) }. import mymod from would then correctly import the module.exports value.

@weswigham
Copy link
Member

weswigham commented Nov 7, 2017

@weswigham that's fair; all I'm saying is that whatever the loader does, import Foo from 'path'; import * as Module from 'path'; Foo !== Module && Module.default === Foo && Object.isFrozen(Module) && typeof Module === 'object' must hold; what the values of Foo, or the string keys and values of Module are, is up to the loader.

First, on the behavior of our downlevel esm loader polyfill: With the linked PR and its associated flag on (to make us do same thing babel does), Foo !== Module would be true, Module.default === Foo would be true, and typeof Module === 'object' would be true, while Object.isFrozen(Module) would be false because neither babel nor us use Object.freeze by default, since the runtime cost of polyfilling it is enormous, and even on runtimes that support it, userland objects which are frozen are usually slower. (Naturally, we've always said that if you absolutely must have the freeze behavior at runtime, you can overwrite our runtime helper with one which calls freeze on the result, and in babel there's a flag to use such a helper in place of the normal one)

Secondly, neither Foo !== Module nor Module.default === Foo are required invariants per spec. The spec only says that, in this case, Module must be a namespace record - it does not, however, preclude Foo from also being an object of the same namespace record, therefore under a given module loader it is very possible for both of those to be false, thus is not a good assumption to make. While there is not syntactic way to receive a self-reference within a module, the loader itself has no such restriction, and it is free to warp the shape of what it imports (including adding a self-reference as the default) at will.

What does TypeScript do right now in the above code if I replace 'path' with 'react'?

Assuming the context of the linked PR where we align with babel, none of the above, since react's top-level export will have an __esModule member at runtime, thanks to this PR and this root file. Meaning it will have no default, as at runtime we will pretend it is an ESM (meaning we assume that the loaded JS has already been transpiled into something approximating a namespace record). Incidentally, this means that import React from "react"; would be a runtime error (unless React is somehow actually exporting a self reference as their default for back-compat since they've changed to using esm internally), while import * as React from "react"; would be correct. Again, this only works because all major real polyfill implementations of an ESM loader defer module errors, as it is impractical not to do so.

@ljharb
Copy link
Contributor

ljharb commented Nov 7, 2017

@weswigham

it does not, however, preclude Foo from also being an object of the same namespace record, therefore under a given module loader it is very possible for both of those to be false, thus is not a good assumption to make.

Can you provide some code that would make the ModuleRecord for a module be === to its own default export? If that code uses ESM, could you provide it with CJS?

Separately, this file actually ensures that it will not have an __esModule member at runtime.

@guybedford
Copy link
Contributor

@ljharb this is possible to do with live bindings let module = createDynamicModule(['default'], function execute () { module.reflect.exports.default.set(module) }). Alternatively just import * as x from 'own-module-name-self-reference'; export default x.

It may be worth React including the __esModule attribute for named export support in interop.

@ljharb
Copy link
Contributor

ljharb commented Nov 7, 2017

Fair enough; but that's still not up to the loader - it's up to the module.

@Jessidhia
Copy link

// self.mjs
import * as namespace from './self'
export { namespace }

namespace.namespace === namespace

This looks like a highly specific corner case, though, and most certainly does not imply that namespace[keyname] !== namespace is false unless you purposely go out of your way to make it so by setting it to be the namespace itself.

namespace[keyname] === namespace is a highly specific exception and a code smell, and not something you should consider as valid or desirable.

(I don't even know if this would work or if it would throw at runtime)

@weswigham
Copy link
Member

The point is simply that that is not an invariant any spec asserts, nor one anyone should rely on, as it is simply untrue.

And @ljharb, the line let module = createDynamicModule(['default'], function execute () { module.reflect.exports.default.set(module) }) is part of the source of a theoretical node custom loader, using the currently exposed API - not the source of an esm itself. So yes, you can construct a loader which does odd things like restructures imports (custom loaders wouldn't really be useful for node if they couldn't).

My point is that no consumer can make sane assumptions about es6 imports without also making assumptions about the underlying loader's semantics. Babel and TS have a loader polyfill with certain loose semantics people have come to expect, while browsers have a very strict loader with ties to the document model and its execution goals, while node (experimental) has its own loader, and none of these are the same right now. ES6 modules are looking like they may become the least portable modern js you can write, since there's a good chance that an import you write for one runtime won't work in another, at least not without a transpilation tool or custom loader to translate. My only goal right now is to align with babel, since it's loader polyfill is a community winner (I mean, we half supported it two years ago already) in order to remove one instance of the competing standards problem from the equation. If node also manages to get its ducks in a row and find a solution that allows it to adopt similar semantics, then everyone but browsers will at least be using the same or similar loader semantics, and hopefully we cut the number of common variations to two.

Re @guybedford:

It may be worth React including the __esModule attribute for named export support in interop

Their index require's an esm-transpiled module and sets it as the module exports object. That import will have an __esModule marker as transpiled esm usually gets one, ergo when that import is assigned as the module exports object, the module will have an __esModule marker. So they should already be good. Probably. Not sure what that conditional .default thing is working around, so I'm not sure if it'd have the marker or not.

@ljharb
Copy link
Contributor

ljharb commented Nov 7, 2017

@weswigham i will concede that “Foo !== Module” isn’t a guarantee provided by the spec. However, the characteristics of the Modue Record object (Module) are guaranteed, and no loader is permitted to violate them - if node has a proposed loader that does, then that won’t end up being able to ship.

The conditional default thing is explicitly to unwrap any Babel interop if it exists, so that CJS can require the object directly.

@guybedford
Copy link
Contributor

@ljharb the characteristics are guaranteed by v8 - Node can't override them even if it wants to.

@ljharb
Copy link
Contributor

ljharb commented Nov 7, 2017

So then the question is, what happens now in typescript when you import the module record and the default export from ‘react’? What are the values of Module and Foo?

@guybedford
Copy link
Contributor

@ljharb it depends if NodeJS will support __esModule (and accept CJS executes early in the pipeline) or not, as it has the ability to set the precedent here. But if React does not export __esModule, then React is only equal to ModuleNamespace({ default: React }).

@aluanhaddad
Copy link
Contributor

But if React does not export __esModule, then React is only equal to ModuleNamespace({ default: React }).

Based on react/src/React.js#L34 and react/src/React.js#L75

That seems to be the intent of the library.

@ljharb
Copy link
Contributor

ljharb commented Nov 8, 2017

@guybedford node will not support that; runtime determination doesn't provide the ordering guarantees that the committee intends (and if needed, the spec will be reworded after this month's meeting to ensure that this is mandated).

@guybedford
Copy link
Contributor

guybedford commented Nov 8, 2017

@ljharb CommonJS is a separate parse goal whose execution semantics are not defined by the specification for interop. There is a TC39 agenda item to discuss this at the coming meeting - it is not black and white by any means. You will get a chance to discuss this further at the end of the month, and I hope you will think a little more about what your stance is here. I personally believe this sort of a tradeoff would be a worthy one in the name of the health of the ecosystem.

@ljharb
Copy link
Contributor

ljharb commented Dec 23, 2017

@guybedford the discussion we had at TC39 around CJS was effectively that ordering must be maintained; as far as I'm aware, the situation with what node will do with CJS, and thus what TypeScript should do, has not changed (and I believe, will not).

@guybedford
Copy link
Contributor

@ljharb yup agreed this is the future now. I hope this gives TypeScript the confirmation it needs now to properly fix default exports in CommonJS interop.

@mhegazy mhegazy added the Fixed A PR has been merged for this issue label Jan 9, 2018
@guybedford
Copy link
Contributor

Just a heads up - I will likely be opening a new issue in the coming months to discuss native NodeJS default export form interop support in TypeScript. If there's an issue already tracking this please let me know too.

@weswigham
Copy link
Member

weswigham commented Jan 9, 2018

@guybedford It'd just be the new esModuleInterop mode with namespace and named-member imports forbidden on modules which have a "synthetic" default, right? AFAIK every node proposal to actually interpret the __esModule marker and create named imports for CJS in ESM officially has fallen through.

@guybedford
Copy link
Contributor

@weswigham exactly I guess it is to use synthetic defaults on CommonJS modules and namespaces on ES modules, where the format for which way to go on this is determined by the NodeJS resolver.

@guybedford
Copy link
Contributor

Although synthetic defaults still seem to work side-by-side with namespace interpretations. It would have to be a form of synthetic that disallowed non-default named exports entirely.

@weswigham
Copy link
Member

@guybedford for my own reference for such a future flag, I should note that import d from "cjs";, import {default as d} from "cjs";, and import * as o from "cjs"; const d = o.default; should all be valid (and identical) in such a scheme, so it's not just some simple syntactic ban on no default forms.

@guybedford
Copy link
Contributor

Sure thanks @weswigham for noting it. It's good to prepare for this to dampen the landing as modules hit platform reality.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Committed The team has roadmapped this issue Domain: ES Modules The issue relates to import/export style module behavior Fixed A PR has been merged for this issue Suggestion An idea for TypeScript
Projects
None yet
Development

Successfully merging a pull request may close this issue.