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

Add command line flag to allow synthetic default exports #5577

Merged
merged 6 commits into from
Nov 26, 2015

Conversation

weswigham
Copy link
Member

From #5285.

When --allowSyntheticDefaultImports is specified, it indicates that the module loader performs some kind of synthetic default import member creation not indicated in the imported .ts or .d.ts. We infer that the default member is either the export= member of the imported module or the entire module itself should that not be present. system has this flag on by default, however it can be overridden with allowSyntheticDefaultImports: false.

@@ -46,6 +46,7 @@ namespace ts {
const compilerOptions = host.getCompilerOptions();
const languageVersion = compilerOptions.target || ScriptTarget.ES3;
const modulekind = compilerOptions.module ? compilerOptions.module : languageVersion === ScriptTarget.ES6 ? ModuleKind.ES6 : ModuleKind.None;
const allowSyntheticDefaultImports = compilerOptions.hasOwnProperty("allowSyntheticDefaultImports") ? compilerOptions.allowSyntheticDefaultImports : modulekind === ModuleKind.System;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about compilerOptions.allowSyntheticDefaultImports !== undefined or typeof compilerOptions.allowSyntheticDefaultImports !== "undefined"?

i am just concerned about readability, and refactor/rename support in tooling.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair! I'll use typeof.

@mhegazy
Copy link
Contributor

mhegazy commented Nov 9, 2015

👍

@mhegazy
Copy link
Contributor

mhegazy commented Nov 9, 2015

One thing to note, with Babel dropping support for export default magic: babel/babel#2212, do we still need this? i would expect systmjs/es6_module_loader to follow suite.

@myitcv
Copy link

myitcv commented Nov 10, 2015

Also related systemjs/systemjs#899

@ahejlsberg
Copy link
Member

My understanding is that Babel will be removing it on the export side, but will be keeping the magic on the import side. SystemJS likewise has magic lifting on the import side. So, unless both Babel and SystemJS both drop the magic on the import side, we still need the flag.

@weswigham
Copy link
Member Author

So @mhegazy, do we want to merge this?

@@ -281,9 +281,14 @@ namespace ts {
description: Diagnostics.Disallow_inconsistently_cased_references_to_the_same_file
},
{
name: "allowSyntheticDefaultImports",
type: "boolean",
description: Diagnostics.Allow_default_imports_from_modules_with_no_default_export
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to add more description that we are not adding anything to the emitted code, but that it is just an illusion to match what the loader would do.

@mhegazy
Copy link
Contributor

mhegazy commented Nov 26, 2015

looks good, if you can to update the flag message that would be great.

@mhegazy
Copy link
Contributor

mhegazy commented Nov 26, 2015

We talked about making it an error to call the namespace import if this flag is on, as es6-module-loader copies the properties on it at run time, but not the prototype. this is however dependent on the implementation of the loader, it is possible that the loader changes the implementation to allow both to be callable.

weswigham added a commit that referenced this pull request Nov 26, 2015
Add command line flag to allow synthetic default exports
@weswigham weswigham merged commit 38215c6 into microsoft:master Nov 26, 2015
@mhegazy
Copy link
Contributor

mhegazy commented Dec 1, 2015

@weswigham
Copy link
Member Author

@mhegazy done.

@mhegazy
Copy link
Contributor

mhegazy commented Dec 1, 2015

thanks!

@glen-84
Copy link

glen-84 commented Dec 1, 2015

Is this the correct option to use when you want to import modules using the ES6 syntax where the definitions of the modules use exports in the form export = x?

For example, my code is targeting ES6 and I want to make use of the type definitions for Gulp. If I use:

import gulp from "gulp";

I get Module '"gulp"' has no default export.. However, if I set allowSyntheticDefaultImports to true, the error goes away.

I just want to make sure that this is one of the intended uses, as I don't quite understand the option's description.

Thanks.

@mhegazy
Copy link
Contributor

mhegazy commented Dec 1, 2015

You should only use this flag iff your loader will generate a default export for you if one does not exisit, e.g. using systemJs. All the flag does, it is mimics what SystemJs does at run time by creating a fake default export on modules that have the same shape as the module.

if you are not using system JS, and your loader does not do any of these tricks, e.g. AMD or commonJs do not use the flag.

@glen-84
Copy link

glen-84 commented Dec 1, 2015

@mhegazy D'oh. I'm targeting ES6 which is then being loaded by Gulp (via Babel [gulp.babel.js]).

As I mentioned here, I'm unsure as to what my options are. I'd like to use ES6 module import syntax, but then the existing type definitions (on DefinitelyTyped) will not work. Is there any guidance in this regard?

@weswigham
Copy link
Member Author

Have you tried import * as gulp from "gulp"? While in ES6 this technically shouldn't be callable/constructable, with many dts files and commonjs modules, it transpiles to the right thing (var gulp = require("gulp");) and we don't emit errors on constructing/calling it.

@mhegazy
Copy link
Contributor

mhegazy commented Dec 1, 2015

so lets step back a bit, ES6 introduced this new concept of a defualt export. it is a special property on the module. since ES6 modules are not supported by any engine, then there is a pre-ES6 implementation that you are using.

the question is the gulp module at runtime does not have a default property on it. if this is the case, then the fix is to update the typings to reflect that, and you can then import it as import gulp from "gulp";.

if this is not true, you can still use the ES6 imports, you just can not use the default import, i.e. import * as gulp from "gulp".

if it still does not have the default export but your loader adds the it for you, then use this flag to avoid errors at compile time.

@glen-84
Copy link

glen-84 commented Dec 1, 2015

@weswigham I'm not at my PC at the moment, but IIRC, that syntax worked for some modules but not others, which was messy, and not really what I wanted in the first place. I will confirm this tomorrow.

@mhegazy Gulp was a bad example, since I'm actually switching to Gulp 4 which doesn't have any typings yet, but let's just pretend that I'm still using Gulp 3.x for now (because this applies to other modules as well): If I updated the typings from export = x to export default x, wouldn't this break existing code that uses the old-style import x = require("x") imports?

I will have another look tomorrow. Thanks for your help so far.

@mhegazy
Copy link
Contributor

mhegazy commented Dec 1, 2015

import d from "module" imports a property named default from that module.
import * as n from "module" imports all properties from the module. this is the closest equivalent to your import n = require("module").

As i mentioned earlier, default is a new thing in ES6, your existing node modules will probably not have a default export. some loaders, like SystemJs fake that by creating a fake default property on the modules on import if they do not have one.

so in short, if you want to use default, first check the module.js file if it exports a default, check the typing to verify they are accurate, and understand your loader. hope that helps.

@glen-84
Copy link

glen-84 commented Dec 2, 2015

@mhegazy,

Some things work with * as x, and some things don't (f.e. modules that export unnamed functions or object literals). It also changes the usage of the module, because you're essentially prefixing the exports, if I'm not mistaken. So, this is messy.

When I use Babel, I can import all these different types of exports in the same consistent ES6 style. I'm assuming that Babel adds the default export during import, so does this mean that it is in fact appropriate for me to use allowSyntheticDefaultImports, where the synthetic default import is done by Babel during run time?

@mhegazy
Copy link
Contributor

mhegazy commented Dec 2, 2015

yes.

@glen-84
Copy link

glen-84 commented Dec 2, 2015

Thanks, I really appreciate your help.

paullessing added a commit to paullessing/dnd-character-sheet2 that referenced this pull request Jan 11, 2016
Should be fixed with the --allowSyntheticDefaultImports flag, see microsoft/TypeScript#5577
But that's not been released yet so can't use it.
paullessing added a commit to paullessing/dnd-character-sheet2 that referenced this pull request Jan 11, 2016
Should be fixed with the --allowSyntheticDefaultImports flag, see microsoft/TypeScript#5577
But that's not been released yet so can't use it.
@felixfbecker
Copy link
Contributor

Many npm modules use module.exports = which in ES6 is equivalent to import x from 'y' (default import). import * as x from 'y' is something completely different, it results in a plain object that cannot be a function for example. So it would be really nice if TS allowed to use default imports for modules that use module.exports = and exports = typing.

@billti
Copy link
Member

billti commented Mar 10, 2016

You need a loader which loads such CommonJS modules, and creates the expected default property, for this to work at runtime. SystemJS does that, and the --allowSyntheticDefaultImports flag added in this pull request is on by default if your module type is SystemJS.

@felixfbecker
Copy link
Contributor

@billti I would like to use this with NodeJS. When transpiling with Babel, Babel adds this. I would like a flag to tell TS to do it too, because I don't like the import x = require('y') syntax

@RyanCavanaugh
Copy link
Member

I don't understand what the goal of using ES6 syntax is if you're going to use it to do things it explicitly does not support? It just means if you ever do move to a real ES6 module runtime, you're going to be broken in nonobvious ways.

@delebash
Copy link

@RyanCavanaugh I think what @felixfbecker and I also want is just the syntax. For example if I want to use jquery currently I can if I use import $ = require('jquery');. Instead I would just like to use the ES6 syntax of import $ from 'jquery'. I don't want to have type require for libs that don't have a defualt export and ES6 import syntax for those libs that do export a default. I currently use both ES6 import syntax and require syntax in as single file for importing different libs.

As @mhegazy said
import d from "module" imports a property named default from that module.
import * as n from "module" imports all properties from the module. this is the closest equivalent to your import n = require("module").

I would like typescript to look at a lib such as jquery determine if it does not have a default export and translate my import $ from 'jquery' to import * as $ from 'jquery' or import $ = require('jquery') which every is more appropriate

@felixfbecker
Copy link
Contributor

@delebash I now use target: ES6 and then compile with Babel. Works perfectly with import default, in fact, import = require() is not even allowed with target: ES6.

@delebash
Copy link

@felixfbecker Yes this works, but one of the points of Typescript is not having to rely on babel or any other compiler and being able compile/convert it's unique code style to whatever js version and module format that you specify in the tsconfig. This should not change just because we have an new version of javascript, Javascript will continue to evovle ES6,7,8 and Typescript needs to support compiling to past and present versions of js.

@felixfbecker
Copy link
Contributor

@delebash I would argue with that, the main reason I use Babel is because TypeScript only allows async/wait for ES6 but ES6 will not transpile stuff that NodeJS does not support yet, default params for example. In Babel however I can use a preset that fits exactly what I need to transpile.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants