Skip to content
This repository has been archived by the owner on Jan 26, 2019. It is now read-only.

allowSyntheticDefaultImports flag in tsconfig.json #214

Closed
YagoLopez opened this issue Dec 23, 2017 · 21 comments
Closed

allowSyntheticDefaultImports flag in tsconfig.json #214

YagoLopez opened this issue Dec 23, 2017 · 21 comments

Comments

@YagoLopez
Copy link

In "Importing a Component" section in https://github.com/wmonk/create-react-app-typescript/blob/master/packages/react-scripts/template/README.md it says:

This project setup supports ES6 modules thanks to Babel.
While you can still use require() and module.exports, we encourage you to use import and export instead.

For example:

Button.js

import React, { Component } from 'react';

class Button extends Component {
  render() {
    // ...
  }
}

export default Button; // Don’t forget to use export default!

I had problems with this and had to use:

import * as React from 'react';

export default class Button extends React.Component {
//...
}

After some investigation I could use the first way using the flag allowSyntheticDefaultImports: true in tsconfig.json. I thought it could be useful for others to know it. Is it worth to make a pull request to add this to the README.md file?

@DorianGrey
Copy link
Collaborator

Hm... that readme section definitely need some additions.
However, that section should mention that import * as React from 'react'; is required with the typescript version to make things work correctly.
The version mentioned in the current readme only works when using babel in the toolchain, since it adds the required default field to react's export (which uses commonjs format) due to some "compatibility" adjustments regarding ES2015 modules. allowSyntheticDefaultImports: true only allows to make the typescript compiler assume the presence of the default export, but it won't be added in case it's absent.

@YagoLopez
Copy link
Author

Aha, so the way you import modules depends on the create-react-app-ts version, is that correct?
If that's true it should be mentioned in the readme as you say.

@DorianGrey
Copy link
Collaborator

Simplified, it's about create-react-app vs create-react-app-ts. The "original" version uses babel by default, while the latter doesn't. babel takes care of adding the default field to exports in commonjs format. The typescript docs explictly mention that allowSyntheticDefaultImports is only true by default when using the system module format, since for every other case, another transpiler or the module bundler has to take care of adding that kind of "compatibility" layer.

@YagoLopez
Copy link
Author

I see, It's a complicated question. Thanks for taking the time to answer. Anyway in my personal opinion due to the fact that by default Typescript (if I'm not wrong) comes with allowSyntheticDefaultImports = false could be useful to mention it in the docs.

@Hotell
Copy link

Hotell commented Dec 26, 2017

also note that with allowSyntheticDefaultImports:true test won't work if you'll use dynamic import() :(

  • that might be issue with ts-jest...
  • question: is there any way to enable babel to compile ? ( babel has import-dynamic-plugin, which handles this in original create-react-app )

image

with allowSyntheticDefaultImports:false, import() works...

image

@YagoLopez
Copy link
Author

@Hotell interesting corner case. Does your code compile with dynamics imports and allowSyntheticDefaultImports: true and fails with jest tests?

Also I dont know the details of your case but if you want to use babel a quick idea is to use CRA instead of CRA-typescript. Not sure if this has sense in your case.

@Hotell
Copy link

Hotell commented Dec 27, 2017

Does your code compile with dynamics imports and allowSyntheticDefaultImports: true and fails with jest tests?

yup, so we had to switch to System.import so both tests and app works :(

Also I dont know the details of your case but if you want to use babel a quick idea is to use CRA instead of CRA-typescript. Not sure if this has sense in your case.

Doesn't make much sense...would do double physical compilation ( running tsc -w + webpack-dev-server )

@mlwilkerson
Copy link

I just hit a similar situation where the normal build with yarn start or yarn build worked fine, but yarn test failed due to the allegedly missing default export. Adding the allowSyntheticDefaultImports: true to tsconfig.test.json got it working.

mlwilkerson added a commit to FortAwesome/react-fontawesome that referenced this issue Dec 28, 2017
@YagoLopez
Copy link
Author

@mlwilkerson good to know it... Also other question, maybe it should be a new issue, but in my case I had to install react-test-renderer to run snapshot tests with jest. Perhaps that should be mentioned in the test section in the docs.

@Hotell
Copy link

Hotell commented Dec 29, 2017

@mlwilkerson

Adding the allowSyntheticDefaultImports: true to tsconfig.test.json got it working.

That doesn't make any sense, as {allowSyntheticDefaultImports: true} is defined within tsconfig.json which is extended by tsconfig.test.json so it's already set to true => so it doesn't work

@YagoLopez

but in my case I had to install react-test-renderer to run snapshot tests with jest

That's in the documentation so it's not an issue rather documented fact

@YagoLopez
Copy link
Author

@mlwilkerson react-test-renderer is only mentioned when using Enzyme. For Jest+snapshot there isn't any mention to react-test-renderer as dependency.

https://github.com/wmonk/create-react-app-typescript/blob/master/packages/react-scripts/template/README.md#running-tests

Anyway it is a subjective matter. I guess I like this point of view: https://en.wikipedia.org/wiki/Don%27t_Make_Me_Think
😄

@mlwilkerson
Copy link

@Hotell my tsconfig.json does not have {allowSyntheticDefaultImports: true}, and the default is false, so setting it to true in tsconfig.test.json does change the behavior.

@DorianGrey
Copy link
Collaborator

@mlwilkerson ,
@Hotell
I'll see if I can shed some light on this ...

As you already might have figured out from reading the ts-jest docs, it supports post-processing code via babel. This mode is selected automatically in case of allowSyntheticDefaultImports: true, as long as this selection is not explicitly suppressed via a skipBabel flag (see here).
The post-process hook is created here: https://github.com/kulshekhar/ts-jest/blob/36875a405db1aea846615f09bc6bda5efb3ce214/src/postprocess.ts#L59
It results in a transformer as required by jest, see: https://github.com/kulshekhar/ts-jest/blob/36875a405db1aea846615f09bc6bda5efb3ce214/src/postprocess.ts#L18

Using a custom .babelrc is possible. Alternatively, a babelConfig property with the explicit config can be provided. By default, ts-jest only takes care that the module kind is properly translated to commonjs, since the tests have to be executable on node. That's a difference to the start or build command, where the module bundler (here: webpack) takes care of handling static and dynamic imports.

To support one of the flags mentioned above, it would be required to change the jest config generated here. However, at least for the .babelrc, I'm not sure where it attempts to pick it up from or at least searches for it. Would be worth trying to add this flag and see if a .babelrc provided in the root of a generated project would be picked up properly. You might do that locally by modifying the package sources in a generated project. However, using this value by default might cause problems for users that don't want to use babel at all, or are fine with the default settings.

@mlwilkerson , regarding the project you mentioned: I suppose the change listed was required since you used import FontAwesomeIcon from '@fortawesome/react-fontawesome'; for importing from commonjs format without explicitly providing a default export (at least it looks like that in projects index file at that branch), i.e. something like module.exports.default=module.exports (similar to what babel's interop does). In general, allowSyntheticDefaultImports: true only suppresses warnings about the type definitions that do not include a default export, but that doesn't have any effect on the generated output - the ts-jest behavior to use babel as postprocessor in that case is exceptional here.

In general, I would not recommend to test code that includes dynamic import statements with jest - these are intended to be processed by bundlers (or the browser), and not on node.

@YagoLopez
Copy link
Author

YagoLopez commented Dec 31, 2017

To test code with dynamic imports seems problematic indeed. An option could be try to mock dynamicaly loaded code.

@mlwilkerson
Copy link

Yes, @DorianGrey, you're correct about my situation. I'm still somewhat new to navigating the different module formats and loading environments, but yes, I do wonder about needing to change the way we are bundling the react-fontawesome package. The fact that module.exports.default isn't being set has been problematic in other situations as well.

@vict-shevchenko
Copy link

In my codebase, I am using regular imports:

import * as React from 'react';
import * as Validator from 'validatorjs';

And had a situation that app compiled and worked correctly, but jest tests were failing.

I resolved this by modifying jest config in package.json with skipBabel property.
According to this and this

@Hotell maybe this may help.

@mlwilkerson
Copy link

We've decide to get rid of default exports altogether, because there have been too many complications like this.

See also:

And further evidence from how both TypeScript and Rollup have to dance around these default exports, namely:

@YagoLopez
Copy link
Author

YagoLopez commented Jan 20, 2018

This could shed some light for anyone interested. All this kind of small quirks seem to be related to ts-jest. I recommend to read all doc from the begining

https://www.npmjs.com/package/ts-jest#supports-synthetic-modules

EDIT: I wrote this in a hurry whitout having read @DorianGrey's more elaborated answer.

As complement to the thread here is a great article about modules and exports in es6:
http://www.jbrantly.com/es6-modules-with-typescript-and-webpack/

I think the issue can be closed.

mlwilkerson added a commit to FortAwesome/react-fontawesome that referenced this issue Jan 22, 2018
@dosentmatter
Copy link

dosentmatter commented May 17, 2018

@mlwilkerson, don't you need allowSyntheticDefaultImports for image imports to work in jest (conversion to file name)?

Currently, in the starter App.tsx, you have

import logo from './logo.svg';

This goes through webpack file-loader. I think it uses commonjs modules and doesn't have a default export
I think this works because webpack transforms it and default works even though it doesn't have a default export.

All these import methods work as well with logoN becoming a string:

/* tslint:disable */
import logo from './logo.svg';
import * as logo1 from './logo.svg';
const logo2 = require('./logo.svg');

If you add a console.log(), tests will not see the string:

/* tslint:disable */
import logo from './logo.svg';
console.log(logo);

Here I am running jest and changing the file extension .js <-> .tsx (remove public from public render() for this to work). When using .tsx, logo is undefined. When using .js, logo is a string. I think this is because .js goes through babel-jest which wraps module.exports in a default export. However, .tsx goes through tsc and tsconfig.json which doesn't allowSyntheticDefaultImports.
import_svg

If you don't want to turn on allowSyntheticDefaultImports,
what you can do is change the App.tsx to:

import * as logo from './logo.svg';

or

/* tslint:disable */
const logo = require('./logo.svg');

@rmlevangelio
Copy link

rmlevangelio commented Jun 10, 2018

@YagoLopez
I think this occurs because React uses CommonJs modules instead of EcmaScript Modules in the main modules
https://github.com/facebook/react/blob/master/packages/react/index.js#L16

And if you use Babel it converts ES modules to CommonJS in a default preset plugin.
https://github.com/babel/babel/blob/master/packages/babel-preset-env/package.json#L37

more info
https://www.youtube.com/watch?v=8O_H2JgV7EQ
https://hacks.mozilla.org/2018/03/es-modules-a-cartoon-deep-dive/

@YagoLopez
Copy link
Author

Thanks @rmlevangelio. Javascript modules are really confusing. Hope we have a solid standard and straight way to manage modules soon. In the mean time I've opted to not to use synthetic imports in Typescript neither default imports for future projects.

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

No branches or pull requests

7 participants