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

allowSyntheticDefaultImports should be the default? #10895

Closed
egamma opened this issue Sep 13, 2016 · 13 comments

Comments

@egamma
Copy link
Member

@egamma egamma commented Sep 13, 2016

We are investigating into simplifying the getting started experience for JS/ES6 projects.

One obstacle we have found is that users are surprised that Intellisense doesn't when they use ES6 style imports. The remedy is tell them to set allowSyntheticDefaultImports to true and we have a trouble shooting section on this in our docs.

Is there any reason why this setting cannot be true by default for JS projects?

CC @waderyan

@tinganho

This comment has been minimized.

Copy link
Contributor

@tinganho tinganho commented Sep 14, 2016

allowSyntheticDefaultImports is only for module loaders which make some magic bindings with old default export to the new ES6 default export.

A simple node project won't do the magic binding so I think it should be off by default.

@egamma

This comment has been minimized.

Copy link
Member Author

@egamma egamma commented Sep 14, 2016

The scenario is that users want to use ES6 style imports, but some typings for modules do not yet use ES6 style exports.

As an example @waderyan an experienced JS and VS Code user thought Intellisense is broken in JS until we pointed him to this this setting. There is no feedback to the user that there are no suggestions because an imported module doesn't use ES6 exports.

@mhegazy

This comment has been minimized.

Copy link

@mhegazy mhegazy commented Sep 14, 2016

one thing to note, Since there is no native implementation for module loading, if the loader/transpiler does not support this transformation, the es6 code will not work.

@mhegazy

This comment has been minimized.

Copy link

@mhegazy mhegazy commented Sep 14, 2016

If we believe most JS users need this, I have no problems switching the default.

@waderyan

This comment has been minimized.

Copy link

@waderyan waderyan commented Sep 14, 2016

Based on numerous customer observations, I believe this is the case.

@mhegazy mhegazy added this to the TypeScript 2.0.5 milestone Sep 14, 2016
@mhegazy mhegazy added the Bug label Sep 14, 2016
@Ciantic

This comment has been minimized.

Copy link

@Ciantic Ciantic commented Sep 20, 2016

what does this do? I'm having issue with default imports:

import something from "somewhere";
something();

Is transpiled to:

var something = require("somewhere");
something.default();

Where as it should be

var something = require("somewhere");
something();

None of the stuff seems to be working with .default syntax.

Currently my tsconfig.json is:

{
    "compilerOptions": {
        "moduleResolution": "node",
        "module": "commonjs",
        "target": "es5",
        "lib": ["es2015", "es6", "dom"],
        "allowSyntheticDefaultImports": true,
        "sourceMap": false,
        "experimentalDecorators": true,
        "emitDecoratorMetadata": true,
        "jsx": "react",
        "outDir": "buildts"
    },
    "exclude": [
        "node_modules"
    ]
}

The allowSyntheticDefaultImports didn't seem to fix this?

@DanielRosenwasser

This comment has been minimized.

Copy link
Member

@DanielRosenwasser DanielRosenwasser commented Sep 20, 2016

allowSyntheticDefaultImports assumes that some runtime behavior will treat a non-ES module's shape as the default export if one doesn't exist (which SystemJS and Babel do). TypeScript doesn't create a default for you if one doesn't exist.

In general, we should think this through since all of this is subject to change depending on how Node defines their interop behavior. I suspect that in Salsa (our JS editing experience), we should switch the default experience because users are probably using Babel if they are using ES modules.

@tinganho

This comment has been minimized.

Copy link
Contributor

@tinganho tinganho commented Sep 20, 2016

we should switch the default experience because users are probably using Babel if they are using ES modules.

That clarifies things. Thought that this was going to switched on by default on TS as well. I don't know the numbers but I suspect a big bunch of TS devs are node devs, which means no magic default bindings. If this is switched on by default that means many TS users will run into this opposite issue. Opposite meaning importing a module and not working on runtime.

@zhengbli

This comment has been minimized.

Copy link
Contributor

@zhengbli zhengbli commented Sep 22, 2016

Does it make more sense if this is a default option for jsconfig.json files instead of tsconfig.json files? We already have two different sets of default values for them.

@mhegazy

This comment has been minimized.

Copy link

@mhegazy mhegazy commented Sep 22, 2016

Does it make more sense if this is a default option for jsconfig.json files instead of tsconfig.json files? We already have two different set of default values for them.

The only numbers that make sense in computer science are 0, 1, and Infinity. we are past 0, and 1, so i guess we have some room to add more defaults here :)

@niieani

This comment has been minimized.

Copy link

@niieani niieani commented Sep 26, 2016

I think this will cause more confusion than good. allowSyntheticDefaultImports allows invalid ES6 code to be treated as valid in case packers/loaders do magical .default imports. I vote against making this default, please keep this optional and only for those who know what they are doing.

@zhengbli

This comment has been minimized.

Copy link
Contributor

@zhengbli zhengbli commented Sep 27, 2016

The change only affects the default value of jsconfig.json, which is more likely to be a pure javascript project. The assumption is that if the user working on the js project writes ES6 modules, he/she is likely using babel or module loaders like systemjs, which are already supporting this syntax. TS projects are not affected at all.

Node users, however, may be surprised if they use the syntax and things break at runtime. Though it is a debate between the perception of "js intellisense if broken" and "this specific feature is assuming the wrong thing for some users", I would think the former does more harm.

@bradleyayers

This comment has been minimized.

Copy link

@bradleyayers bradleyayers commented Sep 28, 2016

I would be satisfied if allowSyntheticDefaultImports was suggested in error messages that stem from modules not having a default export. In my case I didn't realise allowSyntheticDefaultImports even existed — had I realised, the fix would indeed have been simple.

Also perhaps it makes sense for something like ts-loader (webpack) to override this value since webpack will do the magic transformation.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
9 participants
You can’t perform that action at this time.