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

SystemJS and default import with export = proposal #5285

Closed
nycdotnet opened this issue Oct 16, 2015 · 22 comments
Closed

SystemJS and default import with export = proposal #5285

nycdotnet opened this issue Oct 16, 2015 · 22 comments
Labels
Committed The team has roadmapped this issue Fixed A PR has been merged for this issue Suggestion An idea for TypeScript

Comments

@nycdotnet
Copy link

I was recently running into some trouble with getting SystemJS to load jQuery using the ES6-style syntax.

import * as $ from 'jquery';

The above code worked fine with RequireJS and AMD format, and when using SystemJS as the loader when my code was emitted by TypeScript in AMD format.

However I noticed odd behavior when emitting this code using the system format and using SystemJS. I was getting all of the static functions of $, but $ itself was an inert object (not a function). So at runtime, $.isArray([]); would work, but $('#myDiv') would throw "$ is not a function".

Here's my original issue on the SystemJS repo: systemjs/systemjs#844

I kept digging around in the SystemJS documentation and eventually noticed that it referenced using a default ES6 import with jQuery:

import $ from 'jquery';

Sure enough when I tried this, it worked, even though jQuery itself does not export a property default.

After more research, I found this on the SystemJS site:

https://github.com/systemjs/systemjs/blob/master/docs/module-formats.md#inter-format-dependencies

When loading CommonJS, AMD or Global modules from within ES6, the full module is available at the default export which can be loaded with the default import syntax.

So when using SystemJS, the "is a" is made available through a virtual default export.

However, this is a problem since there's not currently a way to model this in TypeScript without breaking existing code using import $ = require('jquery') syntax.

Current jQuery definition on Definitely Typed:

declare module "jquery" {
  export = $;
}

Proposal

In light of the documented behavior of SystemJS, I'd like to propose a change to TypeScript to allow ES6-style default import syntax to work with an ambient external module declaration that uses export = syntax, as long as the module format is "system".

With this change, this code would compile cleanly if TypeScript is set to "system", but would still error using "amd", "commonjs" or "umd" with "Module 'my-module' has no default export":

my-module.d.ts

declare module "my-module" {
  var doStuff : () => void;
  export = doStuff;
}

test.ts

import ds from "my-module";
ds();

Related:
#4337
#2719
CC: @vvakame @basarat @johnnyreilly @jbrantly @guybedford

@myitcv
Copy link

myitcv commented Oct 16, 2015

Here's my understanding of the problem.

import * as $ from "jquery";

console.log($ === window.$);

will output false because the import * returns an ES6 module namespace object when using SystemJS (jquery is, as you say a global in this respect because it pollutes the global namespace)

So the type definition needs to be something like:

declare module "jquery" {
  global.$ = ...;
}

The above code is not valid but is a proposal here

I'm hoping that #4665 is also going to address how to make the following a compile error given such a type definition:

import * as $ from "jquery";

(unless of course $ is explicitly exported as a non-global in the definition of "jquery" or equivalent)

But equally throw compile errors when $ is used in files which are missing import .... "jquery"

@nycdotnet
Copy link
Author

@myitcv I wasn't talking about global scope here, at least not intentionally. jQuery rolls its own UMD and I was intending to take advantage of that.

To eliminate global pollution as a source of this confusion, instead of $, it's possible to alias jQuery as jq in the import. This should demonstrate that it really is loading as an external module and not just as a coincidence.

import * as jq from 'jquery';
console.log(jq.isFunction(jq)); // true with --module amd or commonjs, false with --module system

The above ES6-style import "works correctly" (logs true) when using SystemJS as the module loader when TypeScript is set to amd or commonjs module style, but fails (logs false) when TypeScript is set to system module style because jq becomes an inert module object and not the jQuery function. Using SystemJS with jQuery mandates using the (technically wrong since there isn't a default) ES6 default import code import jq from 'jquery';.

The ES6 default import code is currently a TypeScript error. My proposal is to allow it when the module style is system since this is actually what is required based on how SystemJS works.

Edit: I previously stated that import jq = require('jquery'); would work, but this doesn't work either. jq is still not a function.

@jbrantly
Copy link

I run into a similar issue when using Babel to transpile since Babel does the fancy import checking which allows something like import $ from 'jquery'. I would like to see this solved generally (not just for SystemJS), perhaps with another flag (that is also turned on by default when using --module system).

@myitcv
Copy link

myitcv commented Oct 17, 2015

@nycdotnet - got it, thanks for clarifying. The more I learn about modules, the more I realise how much I don't know....

Indeed I ran into what I think is a similar problem. I'm outputting es5 and system.

But my conclusion wasn't as complete as yours:

My proposal is to allow it when the module style is system since this is actually what is required based on how SystemJS works.

👍 from me on this proposal

@nycdotnet
Copy link
Author

Yes you hit the same issue with d3 and SystemJS. It seems a bit of improved documenation for importing modules that don't provide a default with SystemJS may be in order at a minimum.

Hey, we're all learning! This just came to my attention while putting together some training content - so I had to keep staring at it instead of living in my happy RequireJS bubble. This is new stuff for everyone. :)

@bryanforbes
Copy link
Contributor

To me, this seems like a problem to be solved at the library level rather than by a loader trying to be "smart" about it. I just ran into this in a project: we're compiling to CommonJS but loading things with SystemJS (Angular 2 application). I'm unable to use the default export because SystemJS blows away my default export. There's no reason for that because I'm writing an ES6-compatible CommonJS module, but because the loader guesses wrong about what I'm actually trying to do I can't. This means that libraries written for use with either SystemJS or an AMD loader won't be able to use default exports when loaded with SystemJS, which is quite silly.

@kitsonk
Copy link
Contributor

kitsonk commented Oct 19, 2015

Shouldn't this be a loader feature flag, especially when the loader introduces a breaking change trying to "help" people. TypeScript chasing after a moving target of SystemJS to the detriment of everyone who isn't using SystemJS seems non-sensical.

@jbrantly
Copy link

Shouldn't this be a loader feature flag, especially when the loader introduces a breaking change trying to "help" people. TypeScript chasing after a moving target of SystemJS to the detriment of everyone who isn't using SystemJS seems non-sensical.

To be fair, it's not just SystemJS. Like I mentioned earlier, it's Babel as well. Even if it was a feature flag many people would turn it on because it's a useful feature and you'd be right back in the same boat with TypeScript not really supporting the paradigm well.

The ES6 default export is meant to (sort of) take the place of CJS export =. In that light, it (sort of) makes sense that loaders/transpilers do this magic import.

@bryanforbes
Copy link
Contributor

If you think of the default export that way, yes it sort of makes sense. The problem is that it's not really like it at all. I can have the following module:

export class Column {
}

export default class Grid {
}

And import things in the following ways:

import Grid, { Column } from "./Grid"; // this is just sugar for the next line
import { default as Grid, Column } from "./Grid";
import * as grid from "./Grid"; // then use grid.default or grid.Column

The third import is the closest to a pure CommonJS module import. But in ES6, there's no way to completely redefined the module's value like in AMD and Node's implementation of CJS.

Using the module above, I can compile it to UMD (so it can be used in node, AMD, and [supposedly] SystemJS and Babel). The problem that @nycdotnet is having is that since SystemJS and Babel clobber default, I won't be able to import the Grid class. I understand the need/want to use modules like jQuery with SystemJS and Babel, but by making the default clobbering behavior non-configurable they have essentially made it so ES6-compatible CJS, UMD, and AMD modules cannot be used.

It should also be noted that CJS never supported module.exports =. The ability to assign to exports was an addition by Node. Technically CJS modules are completely ES6 compatible, however Node's extension allows them to be incompatible.

@jbrantly
Copy link

@bryanforbes I don't disagree with any of your points. The reality, though, is that this is currently a pain point. We are in a CJS -> ES6 transition phase. We want to write ES6 modules now and still be able to consume CJS (since most modules published to NPM are CJS). People are bound to build bridges. Since there is not a 1 to 1 mapping some decisions have been made with tradeoffs. That is the reality. It would be nice if TypeScript easily supported this reality with its definitions. This proposal is one way to help with that (maybe not the best way, but a way). At the very least it illustrates the issues.

It's easy for us already in TypeScript-land to want to remain pure, but this can be a blocker to adoption. People are using SystemJS and Babel already (because they actually solve issues/make development easier), and they go to introduce TypeScript and run into stuff like this.

@bryanforbes
Copy link
Contributor

@jbrantly It sounds like you and I, for the most part, are on the same page. I see the issue and it's definitely a problem. I think what's giving me pause is that while SystemJS and Babel have "solved" the issue in the short term for CJS, AMD, and UMD modules that existed before ES6 modules (like jQuery), they have broken CJS, AMD, and UMD modules written with ES6 in mind. I'd rather have a solution that works for everyone rather than one that only works for older modules and new format (SystemJS and ES6) modules.

@jbrantly
Copy link

@bryanforbes Yea I don't think we're far apart. I think your issue is more with SystemJS. All I'm advocating for is a way for TypeScript to easily support definitions for libraries that are multi-targeted (and use a default export and/or export =). How they got multi-targeted is of less concern to me, but obviously popular ways are SystemJS or Babel 😁

@nycdotnet
Copy link
Author

For the record - I agree that a better solution would be a way to be able to specify in the SystemJS config "don't auto-detect default", and then to have a way to also specify "allow this ES6 import to be an 'is a' instead of just 'has a'" so that import * as $ from 'jquery'; would allow $ to be a function and not just an inert object with properties.

I'm glad I'm not the only one running into this. Relaxing this rule in TypeScript would be a convenient "quick win", but I admit it could certainly lead to a chase if the default feature were ever removed (we're perhaps already in that situation with defaultJSExtensions and TypeScript). It would be nice to have some statement from the SystemJS team on that, but of course WHATWG might have the final say anyway.

@guybedford
Copy link
Contributor

import * as $ from 'jquery' can only ever be a module namespace object by the ES modules spec. It is not advisable to alter that behaviour as such code would never work in true ES module environments.

Simply adding the exports.__esModule = true flag to a CommonJS or AMD module is enough to make things work as expected (it informs SystemJS and Babel output the whole object is a namespace object, instead of a default export). Babel already does this, it could well be worth outputting such a flag from TypeScript to make these sorts of interops work out.

@nycdotnet
Copy link
Author

Thanks for weighing in.

import * as $ from 'jquery' can only ever be a module namespace object by the ES modules spec.

This is true. However, the current semantics of TypeScript allow this to work when transpiling to CommonJS or AMD format. The value brought into the local $ variable is the jQuery function in this case when the emitted JavaScript is loaded with Node or RequireJS (and even when loaded via SystemJS when the TypeScript is compiled to commonjs or AMD format).

SystemJS does not allow this syntax to work. $ will not be a function, but will have all of its properties. This is annoying from an "I want my stuff to work" perspective, but is completely understandable because it tracks the actual ES6 spec where the $ should be an inert module namespace object (as you've said).

SystemJS does permit the default import syntax with ES5 libraries. However, this is equally incorrect* because jQuery doesn't have a default export. To allow the default import syntax to work with TypeScript 1.6 and SystemJS, we need to modify the type definition of jQuery to make it "lie" that jQuery has a default import even though it doesn't.

*Note: I am saying "incorrect" in the strictest possible way regarding working to the spec - this is not intended to be pejorative or disrespectful of SystemJS or TypeScript in any way. I respect your work greatly.

There's three things here: jQuery (or really any ES5 library), TypeScript and SystemJS.

Since we can't change jQuery, we can only change the TypeScript code that uses jQuery, the TypeScript definition that describes jQuery, or SystemJS's behavior (perhaps via config).

I don't think it's a good idea to change the jQuery definition because we still need it to work in the CommonJS and AMD scenarios.

I've proposed a change here to allow TypeScript to work with SystemJS's behavior of automatically nominating a default export with ES5 libraries. This allows us to keep the TypeScript definition the same (we don't have to "lie" and say that jQuery has a default export when it doesn't) and it just means that when using SystemJS we have to use the default import syntax to consume a library that "is a" even though there technically isn't an export named default.

I suppose an alternate approach could be a new meta property in the SystemJS config to make it allow particular imports to "be a" instead of strictly conforming to the spec and having the object always come in as a module namespace object. That would allow people to opt-in to allowing the import to be a function for particular libraries only.

@guybedford
Copy link
Contributor

The strictest way to support interop is to treat all global, CommonJS and AMD modules as the module namespace object with a single default export taking the value of the module - { default: module.exports }. That supports all cases without further configuration necessary to understand when we can and can't support the export.name = 'value' CommonJS style. The extension of the namespace object with iterated exports from this default is then an additional feature to support named exports on top of that, which I appreciate makes it difficult to understand how the interop really works.

A configuration option in SystemJS to inform that the module.exports should be directly iterated and converted into a namespace object itself could well be possible (effectively exports.__esModule by metadata).

I must admit my knowledge of TypeScript is quite limited, so that I'm not in a position to comment on the proposal above. Just let me know if I can provide any further help here at all.

@bryanforbes
Copy link
Contributor

@guybedford Thanks for weighing in. I either didn't know or had forgotten about exports.__esModule. Given this flag, I think the following should be proposed:

Module imports

Old Style

Given the following module:

class Thing {
    static staticMethod() { }

    instanceMethod() { }
}

exports = Thing;

It should be importable via the following syntax:

import Thing from "./thing";
import Thing, { staticMethod } from "./thing";
import * as thing from "./thing"

// The third import could then be used as such:
var myThing = new thing.default();
thing.staticMethod();

ES6 Style

No change would be necessary to the module import semantics

Code Generation

Old Style

Code generation for old style modules would remain the same, however code generation for importing old style modules and old style ambient declarations would change. Given the following imports of an old style module:

import Thing, { staticMethod } from "./thing";

new Thing();
staticMethod();

As well as the following:

import * as thing from "./thing";

new thing.default();
thing.staticMethod();

Compiling to CommonJS, AMD, or UMD would produce JavaScript similar to the following:

var Thing_1 = require("./thing");

new Thing_1();
Thing_1.staticMethod();

The output for compiling to SystemJS or ES6 would remain the same.

ES6 Style

Code generation for ES6 style modules would need to be changed to add exports.__esModule = true to the generated JavaScript. Given the following module:

export default class Thing { }

export function thing () { }

Compiling to CommonJS, AMD, or UMD would produce JavaScript similar to the following:

var Thing = (function () { ... })();
exports.default = Thing;

function thing() { }
exports.thing = thing;

Object.defineProperty(exports, "__esModule", {
  value: true
});

Compiling to SystemJS would need to add the __esModule property as well (@guybedford, please correct me if I'm wrong about this).

Code generation for ES6 style modules would remain the same.

@nycdotnet and @jbrantly, what do you think?

@DanielRosenwasser DanielRosenwasser added the Suggestion An idea for TypeScript label Oct 22, 2015
@ahejlsberg
Copy link
Member

I think one way we could solve this issue is by introducing a -moduleAutoDefault compiler option that would cause modules using export = to automatically be given a default property whose type is the same type as the module itself. Basically, this compiler option would be indication that the module loader performs "auto default lifting" (for lack of a better word) of non-ES6 modules (modules without the __esModule marker) in the manner of SystemJS.

With this compiler option you'd be able to import jQuery either as

import * as $ from "jquery";

or as

import $ from "jquery";

You'd of course also still be able to import individual members

import { ajax, getJSON } from "jquery";

With the -moduleAutoDefault option we could even consider removing the call and construct signatures from a namespace import (the $ produced by the first import above) since, with an auto-default-lifting module loader, those call and construct signatures are only present on the default property. That would cause @nycdotnet's original issue to become a compile-time error.

BTW, I'm totally open to a better name than -moduleAutoDefault.

@nycdotnet
Copy link
Author

Thanks for the reply, Anders. I'm pleased that you'd be open to adding a new switch to accommodate this on the import side. I'll bike-shed --autoDefaultImport as the switch name, or perhaps --abideDefaultImport, just to help clarify that the switch affects the shape of the imported object.

I think it's entirely fair that if such a switch were provided, import * as $ from "jquery"; would cause $("#myDiv"); to be a compile error "Cannot invoke an expression whose type lacks a call signature".

@mhegazy mhegazy added the Committed The team has roadmapped this issue label Nov 7, 2015
@mhegazy mhegazy added this to the TypeScript 1.8 milestone Nov 7, 2015
@mhegazy
Copy link
Contributor

mhegazy commented Nov 7, 2015

@weswigham care to send a PR for this?

@mhegazy mhegazy added the Fixed A PR has been merged for this issue label Dec 1, 2015
@mhegazy
Copy link
Contributor

mhegazy commented Dec 1, 2015

this should be handled with the new --allowSyntheticDefaultImports in #5577

@mhegazy mhegazy closed this as completed Dec 1, 2015
@myitcv
Copy link

myitcv commented Dec 1, 2015

@weswigham @mhegazy - thanks very much for implementing this.

micnigh added a commit to micnigh/boilerplate-isomorphic-typescript that referenced this issue Feb 4, 2016
- `default` export became a requirement recently.  Most typescript library definitions have not been updated with a `default`.  So a flag `allowSyntheticDefaultImports` was added to typescript to ignore errors related to this issue.
- See microsoft/TypeScript#5285
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
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 Fixed A PR has been merged for this issue Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

10 participants